Fiber channel driver for Broadcom/Emulex FC host bus adapters.
Needs ReviewPublic

Authored by ram.vegesna_broadcom.com on Jun 30 2017, 1:11 PM.

Details

Reviewers
ken
mav
Group Reviewers
manpages
Summary

First cut of ocs_fc driver.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13509
Build 13734: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
ken added a reviewer: mav.Jul 3 2017, 8:17 PM
ken added a comment.Jul 3 2017, 8:52 PM

I already sent Ram email about this, from from a purely functional standpoint:

I have been able to get a tape drive / library to probe and run in point to point mode when the library / drive are directly attached to the card. (I have not tried going through a switch.) This is with a Spectra Logic T120 exported via an IBM LTO-5 drive.

I have found several issues with the latest code:

  1. The tape library doesn’t probe in fabric mode when it is direct attached to the card. The driver defaults to fabric mode from what I can tell. Instead, the driver just bounces the link up and down (apparently), but from the traces, there are just 16Gb training sequences going over the wire from the initiator. The tape drive is 8Gb, so nothing really happens. I sent Ram two traces, one from a case where the initiator is just doing a training sequence, and the other from where the tape drive tries to do a FLOGI and then nothing happens after that. In both cases the driver is reporting that the link is going up and down.
  1. Attempting to probe in loop mode (hint.ocs_fc.0.topology=3) when direct attached to the library yields a panic:

ocs_fc0: <Emulex LightPulse FC Adapter> mem 0xde418000-0xde41ffff,0xde420000-0xde420fff irq 32 at device 0.0 on pci2
OCS FC Driver (0.0.9999.0) Copyright (C) 2017 Broadcom. The term ?Broadcom? refers to Broadcom Limited and/or its subsidiaries.
ocs_fc0: Setting topology=0x3
ocs_fc: sli_get_config:3890:ocs_fc0:FW1:11.2.156.27 (2e313536) / FW2:11.2.156.27 (2e313536)
ocs_fc: sli_get_config:3893:ocs_fc0:HW1: 0000000b / HW2: 00000010
ocs_fc: ocs_xport_initialize:455:ocs_fc0:Emulex LightPulse FC Adapter: Can't set the toplogy
ocs_fc0: ocs_pci_attach: failed to initialize transport object
panic: XXX trying to free with un-initialized mtx!?!?

cpuid = 5
time = 1499104391
KDB: stack backtrace:
db_trace_self_wrapper() at 0xffffffff803756bb = db_trace_self_wrapper+0x2b/frame 0xfffffe08b3f58350
vpanic() at 0xffffffff8095569c = vpanic+0x19c/frame 0xfffffe08b3f583d0
panic() at 0xffffffff80955723 = panic+0x43/frame 0xfffffe08b3f58430
ocs_xport_free() at 0xffffffff827b4aa2 = ocs_xport_free+0xa2/frame 0xfffffe08b3f58460
ocs_pci_attach() at 0xffffffff8277f3bc = ocs_pci_attach+0x125c/frame 0xfffffe08b3f584d0
device_attach() at 0xffffffff80989a2e = device_attach+0x3de/frame 0xfffffe08b3f58510
pci_driver_added() at 0xffffffff8065fea9 = pci_driver_added+0xe9/frame 0xfffffe08b3f58550
devclass_driver_added() at 0xffffffff809878ed = devclass_driver_added+0x7d/frame 0xfffffe08b3f58590
devclass_add_driver() at 0xffffffff80987814 = devclass_add_driver+0x144/frame 0xfffffe08b3f585d0
module_register_init() at 0xffffffff8093a2c0 = module_register_init+0xc0/frame 0xfffffe08b3f58600
linker_load_module() at 0xffffffff8092d788 = linker_load_module+0xba8/frame 0xfffffe08b3f58910
kern_kldload() at 0xffffffff8092edc9 = kern_kldload+0xa9/frame 0xfffffe08b3f58950
sys_kldload() at 0xffffffff8092ee8b = sys_kldload+0x5b/frame 0xfffffe08b3f58980
amd64_syscall() at 0xffffffff80db1f69 = amd64_syscall+0x549/frame 0xfffffe08b3f58ab0
Xfast_syscall() at 0xffffffff80d92ecb = Xfast_syscall+0xfb/frame 0xfffffe08b3f58ab0

  • syscall (304, FreeBSD ELF64, sys_kldload), rip = 0x800871a9a, rsp = 0x7fffffffde98, rbp = 0x7fffffffe410 ---

KDB: enter: panic
[ thread pid 9224 tid 102778 ]
Stopped at 0xffffffff80996fcb = kdb_enter+0x3b: movq $0,0xffffffff81887780 = kdb_why

  1. Probing the tape library in point to point mode (hint.ocs_fc.0.topology=2) does work. I can talk to the drive just fine. The FC-Tape flags are turned on in the PRLI. Confirmed Completion is requested by the initiator, so the tape drive is sending completion confirmations. However, the CRN is not getting set; it is 0. That is another component of FC-tape, and needed to make sure commands don’t get out of order. Do you have SRR support? Will the driver send a REC if the tape drive takes too long to respond to a command?

I have confirmed that with the isp(4) driver, unloading the driver when you’ve created CTL LUNs with the driver loaded hangs. So the ocs_fc driver is no different in that regard. It's something that we need to fix with CTL in general.

Is there a way to get the board to start off in point to point mode and fall back to loop?

Starting off in fabric mode with no apparent fallback will likely yield some confused users who don’t know why their tape library isn’t attaching.

I haven’t gotten traces of target mode talking to the isp(4) driver yet.

Hopefully this gives something to go on for now. In summary:

  1. We need to make sure that tape drives probe out of the box, whether direct or fabric attached.
  2. Loop mode shouldn't cause a panic.
  3. The CRN needs to be set properly, and SRR and REC should also be properly supported.
mav added a comment.Jul 4 2017, 5:58 AM

I am happy to see FreeBSD getting one more FC driver. Thank you!

Without being able to try this code (the only Emulex cards I have now are ancient 9000-series which seems not supported here) I've added some comments based on very quick look. If you have some spare card samples for developers, I would happily get one to be able to test/improve CAM/CTL compatibility with it in my further work.

sys/dev/ocs_fc/bus_if.h
1 ↗(On Diff #30250)

I believe this bus_if.h file, same as device_if.h and pci_if.h should not be here. Those are built dynamically from respective .m files, and these don't even belong here.

sys/dev/ocs_fc/ocs.h
220

Those macros look pretty bad to me due to use of specific variable. Beside, since they are used only in few places, IMHO they more obfuscate the code then help.

sys/dev/ocs_fc/ocs_cam.c
415

I suppose here you should notify CAM about initiator gone via xpt_async(AC_CONTRACT), same as above it should be notified for initiator arrival. It allows CTL to clean/update LUN state after initiator gone and/or returned.

587

In case of request handled by wildcard LUN it would be good to set inot->ccb_h.target_id/target_lun to real values to let CAM/CTL know what is the real target. CTL restore original wildcard values when resubmit INOT back.

613

inot->arg seems not set here. Modern FreeBSD supports it too.

1692

It may be not critical, but I would set those three independently of initiator role. I don't know whether your hardware and driver support live role change, but at least with isp(4) driver it works fine. I guess CTL CAM target frontend won't be happy if initiator_id change live.

1706

I have feeling that here must be -1 in case of buffer not aligned to page boundary.

1751

This seems reporting HBA port WWNs, while XPT_GET_TRAN_SETTINGS should report properties of specific target port. HBA port WWNs should be reported via XPT_PATH_INQ cpi->xport_specific.fc.* fields instead.

1910

It would be nice to add XPT_ABORT support for XPT_SCSI_IO too. While CAM does not use it now, I hope it may start at some point. Besides I find it useful for testing target implementation.

1987

It would be nice if code fit into 80 columns when possible.

sys/dev/ocs_fc/ocs_device.c
1742

I don't know whether it causes problem here, but testing isp(4) I've recently noticed that QLogic BIOS does not register FC-4 type, and so does not appear in GID_FT response at least on my old Brocade switch, that made impossible OS booting from FreeBSD isp(4) target.

In D11423#237221, @mav wrote:

I am happy to see FreeBSD getting one more FC driver. Thank you!

Without being able to try this code (the only Emulex cards I have now are ancient 9000-series which seems not supported here) I've added some comments based on very quick look. If you have some spare card samples for developers, I would happily get one to be able to test/improve CAM/CTL compatibility with it in my further work.

Hi Motin,

Thanks for your comments. I will update once I fix them.
I am working on sending you our adapters.

Thanks,
Ram

Fixed following issues as per the review comments:

  1. Added code to set ERP(Error Recovery Protocol) in WQE, This is to tell FW to do timers and issue REC and SRR if target supports RETRY.
  2. Fixed panic which happens when wrong module parameter is provided.
  3. Added code to update CAM about the initiator added/removed.
  4. Fixed sending correct target id when request handled by wildcard.
  5. In XPT_TRANS_SETTING, populate target port wwpn,wwnn.
  6. Added code to support XPT_SCSI_IO handling in XPT_ABORT.
  7. Replaced GIDFT with GIDPT, as some of the vendors are not registering FC-4 type.
In D11423#237113, @ken wrote:

I already sent Ram email about this, from from a purely functional standpoint:

I have been able to get a tape drive / library to probe and run in point to point mode when the library / drive are directly attached to the card.  (I have not tried going through a switch.)  This is with a Spectra Logic T120 exported via an IBM LTO-5 drive.

I have found several issues with the latest code:

  1. The tape library doesn’t probe in fabric mode when it is direct attached to the card. The driver defaults to fabric mode from what I can tell. Instead, the driver just bounces the link up and down (apparently), but from the traces, there are just 16Gb training sequences going over the wire from the initiator. The tape drive is 8Gb, so nothing really happens. I sent Ram two traces, one from a case where the initiator is just doing a training sequence, and the other from where the tape drive tries to do a FLOGI and then nothing happens after that. In both cases the driver is reporting that the link is going up and down.
  2. Attempting to probe in loop mode (hint.ocs_fc.0.topology=3) when direct attached to the library yields a panic:

    ocs_fc0: <Emulex LightPulse FC Adapter> mem 0xde418000-0xde41ffff,0xde420000-0xde420fff irq 32 at device 0.0 on pci2 OCS FC Driver (0.0.9999.0) Copyright (C) 2017 Broadcom. The term ?Broadcom? refers to Broadcom Limited and/or its subsidiaries. ocs_fc0: Setting topology=0x3 ocs_fc: sli_get_config:3890:ocs_fc0:FW1:11.2.156.27 (2e313536) / FW2:11.2.156.27 (2e313536) ocs_fc: sli_get_config:3893:ocs_fc0:HW1: 0000000b / HW2: 00000010 ocs_fc: ocs_xport_initialize:455:ocs_fc0:Emulex LightPulse FC Adapter: Can't set the toplogy ocs_fc0: ocs_pci_attach: failed to initialize transport object panic: XXX trying to free with un-initialized mtx!?!?

    cpuid = 5 time = 1499104391 KDB: stack backtrace: db_trace_self_wrapper() at 0xffffffff803756bb = db_trace_self_wrapper+0x2b/frame 0xfffffe08b3f58350 vpanic() at 0xffffffff8095569c = vpanic+0x19c/frame 0xfffffe08b3f583d0 panic() at 0xffffffff80955723 = panic+0x43/frame 0xfffffe08b3f58430 ocs_xport_free() at 0xffffffff827b4aa2 = ocs_xport_free+0xa2/frame 0xfffffe08b3f58460 ocs_pci_attach() at 0xffffffff8277f3bc = ocs_pci_attach+0x125c/frame 0xfffffe08b3f584d0 device_attach() at 0xffffffff80989a2e = device_attach+0x3de/frame 0xfffffe08b3f58510 pci_driver_added() at 0xffffffff8065fea9 = pci_driver_added+0xe9/frame 0xfffffe08b3f58550 devclass_driver_added() at 0xffffffff809878ed = devclass_driver_added+0x7d/frame 0xfffffe08b3f58590 devclass_add_driver() at 0xffffffff80987814 = devclass_add_driver+0x144/frame 0xfffffe08b3f585d0 module_register_init() at 0xffffffff8093a2c0 = module_register_init+0xc0/frame 0xfffffe08b3f58600 linker_load_module() at 0xffffffff8092d788 = linker_load_module+0xba8/frame 0xfffffe08b3f58910 kern_kldload() at 0xffffffff8092edc9 = kern_kldload+0xa9/frame 0xfffffe08b3f58950 sys_kldload() at 0xffffffff8092ee8b = sys_kldload+0x5b/frame 0xfffffe08b3f58980 amd64_syscall() at 0xffffffff80db1f69 = amd64_syscall+0x549/frame 0xfffffe08b3f58ab0 Xfast_syscall() at 0xffffffff80d92ecb = Xfast_syscall+0xfb/frame 0xfffffe08b3f58ab0
    • syscall (304, FreeBSD ELF64, sys_kldload), rip = 0x800871a9a, rsp = 0x7fffffffde98, rbp = 0x7fffffffe410 --- KDB: enter: panic [ thread pid 9224 tid 102778 ] Stopped at 0xffffffff80996fcb = kdb_enter+0x3b: movq $0,0xffffffff81887780 = kdb_why
  3. Probing the tape library in point to point mode (hint.ocs_fc.0.topology=2) does work. I can talk to the drive just fine. The FC-Tape flags are turned on in the PRLI. Confirmed Completion is requested by the initiator, so the tape drive is sending completion confirmations. However, the CRN is not getting set; it is 0. That is another component of FC-tape, and needed to make sure commands don’t get out of order. Do you have SRR support? Will the driver send a REC if the tape drive takes too long to respond to a command?

    I have confirmed that with the isp(4) driver, unloading the driver when you’ve created CTL LUNs with the driver loaded hangs. So the ocs_fc driver is no different in that regard. It's something that we need to fix with CTL in general.

    Is there a way to get the board to start off in point to point mode and fall back to loop?

    Starting off in fabric mode with no apparent fallback will likely yield some confused users who don’t know why their tape library isn’t attaching.

    I haven’t gotten traces of target mode talking to the isp(4) driver yet.

    Hopefully this gives something to go on for now. In summary:
  4. We need to make sure that tape drives probe out of the box, whether direct or fabric attached.
  5. Loop mode shouldn't cause a panic.
  6. The CRN needs to be set properly, and SRR and REC should also be properly supported.

Hi Ken,

We support following topologies:

hint.ocs_fc.X.topology: FC topology:
• 0 – auto-select; first tries N_Port, then loop. (default)
• 1 – N_Port (point-to-point)
• 2 – loop

N_Port topology covers both Fabric and point-to-point mode. Driver decides the Port type based on the PLOGI response.

In your test (hint.ocs_fc.0.topology=2) means loop topology. So, Tape drive attach worked with loop topology.

Panic with hint.ocs_fc.0.topology=3: Driver initialization failed specifying "unsupported Topology". I fixed the driver panic here.

Yes, we have SRR and REC support. Our Firmware will do REC and SRR and perform the timers. I missed a change here to update the FW that FCP Error Recovery Protocol should be performed. Fixed the issue.

Regarding CRN, After discussing internally in our team, we thought of ignoring it as only few targets support it. Please let know if ignoring may cause any issue.

Task Retry ID - which is used by sequence level error recovery is automatically stamped by our Firmware.

Regarding Tape drive failure with default topology, From the trace I am not able to find the root cause for link toggle.

Thanks,
Ram

ram.vegesna_broadcom.com marked 10 inline comments as done.Jul 14 2017, 8:43 AM
ram.vegesna_broadcom.com added inline comments.
sys/dev/ocs_fc/ocs_cam.c
1692

As of now, we are not supporting live role change.

1987

I will submit the indentation changes in the next update.

sys/dev/ocs_fc/ocs_device.c
1742

Replaced GID FT with GIT PT to support this case.

mav added a comment.Jul 14 2017, 10:12 AM
  1. Fixed sending correct target id when request handled by wildcard.

This is not what I was thinking about. When request is handled by wildcard LUN, CAM and CTL should receive real target and LUN IDs. As I understand, your new code still does not set target_lun value, leaving it wildcard.

By the way, looking wider, I've noticed that your code in ocs_fc_decode_lun() decodes 8-byte LUN into flat space. But there is no reason to do it now. Recent FreeBSD versions support full 8-byte LUNs if PIM_EXTLUNS flag is set. While CTL still does not make use of that, I was thinking about making it support LUN conglomerates at some point.

ram.vegesna_broadcom.com marked 2 inline comments as done.Jul 14 2017, 12:21 PM
In D11423#240075, @mav wrote:
  1. Fixed sending correct target id when request handled by wildcard.

This is not what I was thinking about. When request is handled by wildcard LUN, CAM and CTL should receive real target and LUN IDs. As I understand, your new code still does not set target_lun value, leaving it wildcard.

By the way, looking wider, I've noticed that your code in ocs_fc_decode_lun() decodes 8-byte LUN into flat space. But there is no reason to do it now. Recent FreeBSD versions support full 8-byte LUNs if PIM_EXTLUNS flag is set. While CTL still does not make use of that, I was thinking about making it support LUN conglomerates at some point.

Sorry missed that, Is this what you intended here: I am still not updating the target id as I am not saving that value now, will add that code.

I will do the changes to support EXTLUNS.

  • ocs_cam.c (revision 40610)

+++ ocs_cam.c (working copy)
@@ -586,6 +586,7 @@

if ((lun < OCS_MAX_LUN) && ocs->targ_rsrc[lun].enabled) {
        trsrc = &ocs->targ_rsrc[lun];
} else if (ocs->targ_rsrc_wildcard.enabled) {

+ lun = CAM_LUN_WILDCARD;

        trsrc = &ocs->targ_rsrc_wildcard;
}

@@ -601,6 +602,9 @@

tmfio->tgt_io.app = abortio;

STAILQ_REMOVE_HEAD(&trsrc->inot, sim_links.stqe);

+
+ inot->ccb_h.status = CAM_CDB_RECVD;
+ inot->ccb_h.target_lun = lun;

inot->tag_id = tmfio->tag;
mav added a comment.Jul 14 2017, 4:02 PM

Sorry missed that, Is this what you intended here: I am still not updating the target id as I am not saving that value now, will add that code.
===================================================================

  • ocs_cam.c (revision 40610) +++ ocs_cam.c (working copy) @@ -586,6 +586,7 @@ if ((lun < OCS_MAX_LUN) && ocs->targ_rsrc[lun].enabled) { trsrc = &ocs->targ_rsrc[lun]; } else if (ocs->targ_rsrc_wildcard.enabled) { + lun = CAM_LUN_WILDCARD; trsrc = &ocs->targ_rsrc_wildcard; }

    @@ -601,6 +602,9 @@ tmfio->tgt_io.app = abortio;

    STAILQ_REMOVE_HEAD(&trsrc->inot, sim_links.stqe); + + inot->ccb_h.status = CAM_CDB_RECVD; + inot->ccb_h.target_lun = lun;

    inot->tag_id = tmfio->tag; =========================================

Only one last line would probably be enough. Previous one seems not from here.

mav added a comment.Jul 28 2017, 8:51 AM

As of now, we are not supporting live role change.

It should better be supported. I've tried to enable target mode, but found expected problems: If target mode enabled via device hints, it won't be able to reply to initiators until target mode is enabled in CTL, leading to command timeouts on initiators. Enabling target in CTL after makes it work, but it seems CTL doesn't receive or drops AC_CONTRACT notifications, as result of what ctladm portlist -i does not report connected initiators until they disconnect and reconnect.

mav added inline comments.Jul 28 2017, 9:49 AM
sys/dev/ocs_fc/ocs_cam.c
791

This and above/below looks like leaked debugging.

2155

I suppose here should be called ocs_hw_flush() to allow kernel dumping on FC storage.

2275

This is overly verbose for normal operation.

In D11423#240180, @mav wrote:

Sorry missed that, Is this what you intended here: I am still not updating the target id as I am not saving that value now, will add that code.
===================================================================

  • ocs_cam.c (revision 40610) +++ ocs_cam.c (working copy) @@ -586,6 +586,7 @@ if ((lun < OCS_MAX_LUN) && ocs->targ_rsrc[lun].enabled) { trsrc = &ocs->targ_rsrc[lun]; } else if (ocs->targ_rsrc_wildcard.enabled) { + lun = CAM_LUN_WILDCARD; trsrc = &ocs->targ_rsrc_wildcard; }

    @@ -601,6 +602,9 @@ tmfio->tgt_io.app = abortio;

    STAILQ_REMOVE_HEAD(&trsrc->inot, sim_links.stqe); + + inot->ccb_h.status = CAM_CDB_RECVD; + inot->ccb_h.target_lun = lun;

    inot->tag_id = tmfio->tag; =========================================

Only one last line would probably be enough. Previous one seems not from here.

In D11423#243919, @mav wrote:

As of now, we are not supporting live role change.

It should better be supported. I've tried to enable target mode, but found expected problems: If target mode enabled via device hints, it won't be able to reply to initiators until target mode is enabled in CTL, leading to command timeouts on initiators. Enabling target in CTL after makes it work, but it seems CTL doesn't receive or drops AC_CONTRACT notifications, as result of what ctladm portlist -i does not report connected initiators until they disconnect and reconnect.

In D11423#243919, @mav wrote:

As of now, we are not supporting live role change.

It should better be supported. I've tried to enable target mode, but found expected problems: If target mode enabled via device hints, it won't be able to reply to initiators until target mode is enabled in CTL, leading to command timeouts on initiators. Enabling target in CTL after makes it work, but it seems CTL doesn't receive or drops AC_CONTRACT notifications, as result of what ctladm portlist -i does not report connected initiators until they disconnect and reconnect.

I think if we support the live role change also this issue will happen. As enabling target mode in CTL should be done in any case. So, I think we need to debug why CTL doesn't receive or drops AC_CONTRACT notifications.
Please correct me if I am missing any thing.

mav added a comment.Jul 29 2017, 12:40 PM
In D11423#243919, @mav wrote:

As of now, we are not supporting live role change.

It should better be supported. I've tried to enable target mode, but found expected problems: If target mode enabled via device hints, it won't be able to reply to initiators until target mode is enabled in CTL, leading to command timeouts on initiators. Enabling target in CTL after makes it work, but it seems CTL doesn't receive or drops AC_CONTRACT notifications, as result of what ctladm portlist -i does not report connected initiators until they disconnect and reconnect.

I think if we support the live role change also this issue will happen. As enabling target mode in CTL should be done in any case. So, I think we need to debug why CTL doesn't receive or drops AC_CONTRACT notifications.
Please correct me if I am missing any thing.

I'm sorry, it seems I was wrong about AC_CONTRACT, it does work.

But I still see a problem in fact that target mode is not controller by CTL: when system boots and driver applies target mode tunable, there is no LUNs configured/enabled till ctld start. It makes initiators to see the target which does not respond on requests. I am not sure it is a nice behavior. in case of isp(4) driver FC link remains down until ctld enable the target role when all LUNs are ready. Could you describe how your approach supposed to work in case I miss something?

In D11423#244184, @mav wrote:
In D11423#243919, @mav wrote:

As of now, we are not supporting live role change.

It should better be supported. I've tried to enable target mode, but found expected problems: If target mode enabled via device hints, it won't be able to reply to initiators until target mode is enabled in CTL, leading to command timeouts on initiators. Enabling target in CTL after makes it work, but it seems CTL doesn't receive or drops AC_CONTRACT notifications, as result of what ctladm portlist -i does not report connected initiators until they disconnect and reconnect.

I think if we support the live role change also this issue will happen. As enabling target mode in CTL should be done in any case. So, I think we need to debug why CTL doesn't receive or drops AC_CONTRACT notifications.
Please correct me if I am missing any thing.

I'm sorry, it seems I was wrong about AC_CONTRACT, it does work.

But I still see a problem in fact that target mode is not controller by CTL: when system boots and driver applies target mode tunable, there is no LUNs configured/enabled till ctld start. It makes initiators to see the target which does not respond on requests. I am not sure it is a nice behavior. in case of isp(4) driver FC link remains down until ctld enable the target role when all LUNs are ready. Could you describe how your approach supposed to work in case I miss something?

Yes, you are right. I got it. I will do the necessary changes.
Our approach worked until now because I used to load driver, create LUNs and enable ctl port using script, So initiators used to work with out any issue.

ram.vegesna_broadcom.com updated this revision to Diff 34323.EditedOct 25 2017, 12:29 PM
  1. Added support for live driver role change via CTL.
  2. Removed some info prints.
  3. Did some bug fixes.
ken added a comment.Nov 8 2017, 9:02 PM

I tried this out, and this breaks automatic tape drive recognition if I haven't set the topology to 2 (loop mode). If I set the topology to 2, it sees the tape drive without a problem.

I'm looking at traces, and it still looks like REC isn't getting sent. You can trigger a REC by doing a 'mt fsf' to go to the next filemark, if the next filemark is far enough away. e.g.:

dd if=big_file bs=1m | dd of=/dev/nsa0 bs=128k
mt rewind
mt fsf

I do see that REC is getting sent in the trace you sent me on October 10th. I don't see the FC setup, though, so I don't know whether your initiator / tape drive are doing the same thing as mine.

Thanks Ken. Sorry missed the auto-topology fix in this drop. I will update the code.
I will try to trigger the REC with the commands you provided, will update you the status.

In D11423#270158, @ken wrote:

I tried this out, and this breaks automatic tape drive recognition if I haven't set the topology to 2 (loop mode). If I set the topology to 2, it sees the tape drive without a problem.

I'm looking at traces, and it still looks like REC isn't getting sent. You can trigger a REC by doing a 'mt fsf' to go to the next filemark, if the next filemark is far enough away. e.g.:

dd if=big_file bs=1m | dd of=/dev/nsa0 bs=128k
mt rewind
mt fsf

I do see that REC is getting sent in the trace you sent me on October 10th. I don't see the FC setup, though, so I don't know whether your initiator / tape drive are doing the same thing as mine.

  1. Changed auto topology detection to try FC-AL first and failover to Nport topology.
  2. Fixed issue of not sending REC for no data commands, by setting the Error Recovery bit in command wrb.
ken added a comment.Nov 9 2017, 3:47 PM

The new diffs fixed both issues: auto negotiation with tape drives and sending RECs for non-data commands. I got a trace to confirm that RECs are getting sent during a long-running SPACE command. It looks like the card sends a REC every 4 seconds, which should be fine. (QLogic seems to send them every 3 seconds. I don't think the spec mandates a specific time.)

I'll try to test target mode a little bit today. One other thing that needs to get done is write a man page for the driver. The man page should cover the following topics at least:

  1. General description of the driver.
  2. What hardware it works with.
  3. Driver kernel configuration options (if any).
  4. Loader tunables and sysctl variables. Include information on what the different settings do.

For examples of device driver man pages, see:

share/man/man4/isp.4
share/man/man4/mpr.4
share/man/man4/mps.4

One thing to remember when writing man pages is that every sentence should start on a new line.

Added man page for ocs_fc driver.

wblock added a subscriber: wblock.Nov 17 2017, 4:53 PM
wblock added inline comments.
share/man/man4/ocs_fc.4
50

This whole introductory section is the old style with halting and confusing wording. Please see https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/manpages-sample-structures.html#manpages-sample-structures-section-4 for a suggested newer style.

53

s/FibreChannel/Fibre Channel/

58

Use the serial comma: s/Point-to-Point/Point-to-Point,/

62

Nonspecific "It" here... probably means FC-Tape and would be clearer just saying that. Also, "encompasses" might be a technical term here, but would probably be clearer with just "includes":

FC-Tape includes four elements from the T-10 FCP-4 specification:
76

Another non-specific "it". Does this refer to "link level error recover"?

77

Use the serial comma: s/part/part,/

79

Too many "it"s.

FC-Tape is automatically enabled when both the controller and target support it."
85

s/Driver/driver/
s/the following/these/
s/FibreChannel/Fibre Channel/

89

These would probably look better without the periods.

94

As above, periods are probably not needed.

101

s/the steps below/these steps/

104
Copy this code to a
.Pa Makefile :
111
Replace
.Pa imagename
with the name of the GRP file.
113
Copy the
.Pa Makefile
and GRP file to a local directory
115
Execute
.Cm make
and copy the generated
.Pa ocsflash.ko
file to
118

I think this also needs .Cm

126
Options are controlled by setting values in
132

This type of thing is usually clearer to read when the items are just the description rather than a full sentence. For example,

Enable initiator functionality.
Default 1 (enabled), 0 to disable.

It's not clear to me whether "hint" is necessary or useful.

135
Enable target functionality.
Default 1 (enabled), 0 to disable.
138
Topology: 0 for Auto, 1 for NPort only, 2 for Loop only.
140

As above, but also please start new sentences on new lines.

Link speed in megabits per second.
Possible values include:
141

Needs ending period.

146

As above: skip the introductory part to make it a sentence, and just give the description the reader is looking for anyway. Also, start sentences on new lines.
Not sure about the parameters. Ideally, we avoid showing stuff in quotes to avoid the reader having to ask whether the quotes are part of the parameter.

Port state (read/write).
Valid values are
.Li online
and
.Li offline .
148

As above.

150

As above.

152
Firmware revision (read-only).
154
Adapter serial number (read-only).
156

As above.

158

As above.

160

As above.

162

As above.

ken added a comment.Nov 18 2017, 3:37 AM

Another thing to note here is that the man page has replaced the entire set of diffs that included the code. Both need to be included in the diffs, and the man page hooked up to the build.

Updated makefile as per the review commnets.

ram.vegesna_broadcom.com marked 31 inline comments as done.Nov 18 2017, 2:28 PM

Updated makefile as per the review commnets.

Thanks Warren for the review.

Hi Ken,
Sorry , I messed the diffs update. I uploaded all the code now. Please let me know if you want me to create a new revision.

Thanks ,
Ram

ken added a comment.Dec 11 2017, 8:44 PM

One thing that needs to get fixed is cosmetic. The reported speeds are 10X what they should be.

da45 at ocs_fc0 bus 0 scbus11 target 2 lun 4
da45: <FREEBSD CTLDISK 0001> Fixed Direct Access SPC-5 SCSI device
da45: Serial Number MYSERIAL0004
da45: 16000.000MB/s transfers WWNN 0x2000000e1e222a31 WWPN 0x2100000e1e222a31 Po
rtID 0xef
da45: Command Queueing enabled
da45: 100000MB (204800000 512 byte sectors)
da44 at ocs_fc0 bus 0 scbus11 target 2 lun 3
da44: <FREEBSD CTLDISK 0001> Fixed Direct Access SPC-5 SCSI device
da44: Serial Number MYSERIAL0003
da44: 16000.000MB/s transfers WWNN 0x2000000e1e222a31 WWPN 0x2100000e1e222a31 PortID 0xef
da44: Command Queueing enabled

sa0 at ocs_fc1 bus 0 scbus12 target 0 lun 0
sa0: <IBM ULTRIUM-HH5 G9N1> Removable Sequential Access SPC-4 SCSI device
sa0: Serial Number 101500520A
sa0: 8000.000MB/s transfers WWNN 0x21150090a500520a WWPN 0x21150090a500520a PortID 0x26
sa0: Command Queueing enabled
ch0 at ocs_fc1 bus 0 scbus12 target 0 lun 1
ch0: <SPECTRA PYTHON 2000> Removable Changer SCSI-3 device
ch0: Serial Number 921400520A
ch0: 8000.000MB/s transfers WWNN 0x21150090a500520a WWPN 0x21150090a500520a PortID 0x26
ch0: 10 slots, 1 drive, 1 picker, 1 portal

Hi Ken,

Please clarify,

"da44: 16000.000MB/s transfers"
should be
"da44: 16000.000Mb/s transfers" --> This seems correct to me but camcontrol utility always prints in MB/s transfers.
or
"da44: 1600.000MB/s transfers" --> Do you want me to report this speed?

Thanks,
Ram

In D11423#281162, @ken wrote:

One thing that needs to get fixed is cosmetic. The reported speeds are 10X what they should be.

da45 at ocs_fc0 bus 0 scbus11 target 2 lun 4
da45: <FREEBSD CTLDISK 0001> Fixed Direct Access SPC-5 SCSI device
da45: Serial Number MYSERIAL0004
da45: 16000.000MB/s transfers WWNN 0x2000000e1e222a31 WWPN 0x2100000e1e222a31 Po
rtID 0xef
da45: Command Queueing enabled
da45: 100000MB (204800000 512 byte sectors)
da44 at ocs_fc0 bus 0 scbus11 target 2 lun 3
da44: <FREEBSD CTLDISK 0001> Fixed Direct Access SPC-5 SCSI device
da44: Serial Number MYSERIAL0003
da44: 16000.000MB/s transfers WWNN 0x2000000e1e222a31 WWPN 0x2100000e1e222a31 PortID 0xef
da44: Command Queueing enabled

sa0 at ocs_fc1 bus 0 scbus12 target 0 lun 0
sa0: <IBM ULTRIUM-HH5 G9N1> Removable Sequential Access SPC-4 SCSI device
sa0: Serial Number 101500520A
sa0: 8000.000MB/s transfers WWNN 0x21150090a500520a WWPN 0x21150090a500520a PortID 0x26
sa0: Command Queueing enabled
ch0 at ocs_fc1 bus 0 scbus12 target 0 lun 1
ch0: <SPECTRA PYTHON 2000> Removable Changer SCSI-3 device
ch0: Serial Number 921400520A
ch0: 8000.000MB/s transfers WWNN 0x21150090a500520a WWPN 0x21150090a500520a PortID 0x26
ch0: 10 slots, 1 drive, 1 picker, 1 portal

ken added a comment.Dec 12 2017, 3:00 PM

Hi Ken,

Please clarify,

"da44: 16000.000MB/s transfers"
should be
"da44: 16000.000Mb/s transfers" --> This seems correct to me but camcontrol utility always prints in MB/s transfers.
or
"da44: 1600.000MB/s transfers" --> Do you want me to report this speed?

Yes, the third one is what we need to report. The numbers printed are in mebibytes/sec (2^20 bytes/sec). Back when we wrote it, the name "mebibytes" and the MiB abbreviation didn't exist. :) So a tape drive attached to a Qlogic controller and running at 8Gb, for example, shows:

sa0 at isp0 bus 0 scbus2 target 0 lun 0
sa0: <IBM ULTRIUM-HH5 G9N1> Removable Sequential Access SPC-4 SCSI device
sa0: Serial Number 101300520A
sa0: 800.000MB/s transfers WWNN 0x21130090a500520a WWPN 0x21130090a500520a PortID 0xef

Fixed issue in reporting the transfer speed.