This commit only affects the behavior of XPT_SCAN_TGT/XPT_SCAN_BUS and has no bearing on the behavior of target SIMs. It also is specific to XPORT_SAS, and has no effect on XPORT_FC. I see no reason for it not to proceed.

As I have told, SAS may also work as target, we just miss respective SIM. And the code skipping the initiaror_id during scan allows to not scan own target, if we ever have non-FC targets. What is so difficult in fixing few SAS SIMs to report initiator_id out of the range of valid initiator role targets?

The patch only impacts XPT_SCAN_TGT/XPT_SCAN_BUS operations that don't apply to target SIMs. Again, I see no reason not to proceed.

The patch will make XPT_SCAN_TGT/XPT_SCAN_BUS to scan target they should not.

Initiator ID has no meaning for SAS initiators, your comment about this function scanning targets that it should not within the domain of XPORT_SAS doesn't make sense to me. Numerical bus ID's don't even make sense within SAS topology, and at some point we need to fix all of this to use port UUIDs as the definitive identifier. We also need to fix the entire SIM API and model so that the buses are truly self-announcing and self-identifying, not this crummy hybrid we have of still pretending that the SAS transport is sequentially scanned like parallel SCSI is. But until that happens, what Andriy is proposing is a seat belt for XPORT_SAS initiator SIMs so that they don't shoot themselves in the foot due to the poor API that we've provided.

I agree with everything you say we should do. But we are not there yet, and until that time SIMs have to keep some sort of mapping table for CAM to use. From the CAM point now all target IDs of the SCSI domain the HBA is connected to, including the initiator_id are from that mapping table. I am not sure what exactly makes no sense to you in my descrintion of how things are working for FC now and why exactly the same is not applicable to SAS to add this dirty hack?

I think it's much less likely that a SIM will internally map UINT_MAX to a valid target than what has already been demonstrated where it'll map an errant initiator_id to a valid target.

It is not a problem of UINT_MAX, it is a problem to ignore potentially correct reported value.

@mav , I do not know much about this but I would expect that if an initiator can also be a target then its target ID -- which, if I understand correctly, you suggest to be treated as an initiator ID -- would be from a different namespace than IDs of its targets. That is, I think that for a SAS initiator it is impossible to see itself as a target (where a target ID is typically also some made up number derived from the actual SAS topology).

Also, about your other point, it's not hard to fake an initiator ID in every SAS SIM and all of them (have to) do it already.
But I do not see a reason to keep doing that when it does not have any real use except for introducing occasional bugs.

In D23852#524734, @avg wrote:

I would expect that if an initiator can also be a target then its target ID -- which, if I understand correctly, you suggest to be treated as an initiator ID -- would be from a different namespace than IDs of its targets.

Why? Think about it as parallel SCSI or loop FC, where each port can be both initiator and target, and have ID from some namespace. That is how initiator_id originally appeared in CAM, as I understand. We actually had some parallel SCSI target support in base, it just died with the hardware.

That is, I think that for a SAS initiator it is impossible to see itself as a target (where a target ID is typically also some made up number derived from the actual SAS topology).

I would not call it impossible. It is just not very useful. For QLogic FC IIRC it partially even works, but I prefer it not to rather then try and fail.

Also, about your other point, it's not hard to fake an initiator ID in every SAS SIM and all of them (have to) do it already.
But I do not see a reason to keep doing that when it does not have any real use except for introducing occasional bugs.

As I have told it works reasonably for FC, it was reasonable for SPI, and I see no reason why it can not for other protocols, that would justify protocol specifics into the shared code.

@ken , you've been around the target code for longer time, what do you think?

In D23852#524775, @mav wrote:

In D23852#524734, @avg wrote:

I would expect that if an initiator can also be a target then its target ID -- which, if I understand correctly, you suggest to be treated as an initiator ID -- would be from a different namespace than IDs of its targets.

Why? Think about it as parallel SCSI or loop FC, where each port can be both initiator and target, and have ID from some namespace. That is how initiator_id originally appeared in CAM, as I understand. We actually had some parallel SCSI target support in base, it just died with the hardware.

That is, I think that for a SAS initiator it is impossible to see itself as a target (where a target ID is typically also some made up number derived from the actual SAS topology).

I would not call it impossible. It is just not very useful. For QLogic FC IIRC it partially even works, but I prefer it not to rather then try and fail.

Also, about your other point, it's not hard to fake an initiator ID in every SAS SIM and all of them (have to) do it already.
But I do not see a reason to keep doing that when it does not have any real use except for introducing occasional bugs.

As I have told it works reasonably for FC, it was reasonable for SPI, and I see no reason why it can not for other protocols, that would justify protocol specifics into the shared code.

@ken , you've been around the target code for longer time, what do you think?

I think we should just fix the SIM drivers to not report a value for the initiator ID that is in the range of possible target IDs.

There are transports (e.g. FC) where a device can be the target and initiator at the same time, and having numeric bus/target/LUN IDs will probably continue to be useful for a long time.

Or, if you want to make it really clear, add a flag to the XPT_PATH_INQ CCB, say something like PIM_NO_INIT_ID, that tells CAM that the initiator ID isn't set.

If we put a hack like this in the scan code, it'll continue to encourage new SIM driver writers to do the same thing as the current SIMs that are doing the wrong thing. When people write a new SIM, they largely look at existing drivers for guidance. Let's fix the drivers to do the right thing, so that when people copy them, they get the right behavior instead of it getting magically changed to correct a random value that someone stuck in there.

I am OK with that, but I think it can be too easy to forget, same as wrong value is set without understanding now. I am thinking whether introduction of some special constant value for initiator_id equal to UINT_MAX or something could be more explicit?

In D23852#524939, @mav wrote:

I am OK with that, but I think it can be too easy to forget, same as wrong value is set without understanding now. I am thinking whether introduction of some special constant value for initiator_id equal to UINT_MAX or something could be more explicit?

Well, we already have CAM_TARGET_WILDCARD, which is used in a number of places in the transport layer, and is defined here:

cam.h:#define CAM_TARGET_WILDCARD ((target_id_t)~0)

We shouldn't use that, since it already has a specific meaning. I guess we could go with CAM_TARGET_WILDCARD - 1 if you wanted to use a special value instead of a flag.

FC isn't part of the problem here, so I ask that we stop reverting to it in the discussion.

Initiator_id simply has no meaning in XPORT_SAS. We can "fix" the drivers in the tree to assign a bogus value, but we're still perpetuating an incorrect and confusing operational myth. My suggestion is that we just fix the various places in the transport that look at initiator_id and have it simply be ignored for XPORT_SAS. That'll be different from what Andriy originally suggested, but more correct than inventing a new magic/bogus value out of thin air. If a SIM wants to reserve a segment of target ids that it would rather not send to its hardware, then it can easily do that for itself.

I'm not against sweeping the tree and making changes to existing drivers, but I want the code in CAM to stop behaving in truly mysterious ways.