| Message ID | 20230522112751.266505-1-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-scsi: Use correct device no in ata_find_dev() | expand |
On 22/05/2023 12:27, Damien Le Moal wrote: Hi Damien, Our mails just crossed... > For non-pmp attached devices managed directly by libata, the device > number is always 0 or 1 and lower to the maximum number of devices > returned by ata_link_max_devices(). However, for libsas managed devices, > devices are numbered up to the number of device scanned on an HBA port, It's not really clear to me which number you mean. For libsas and lib ata, ata_device->devno is configured the same, it's just that the sdev per ata_device does not have the same numbering scheme for libsas. For libsas - or scsi_transport_sas, to be more exact - the sdev id is per shost. > while each device has a regular ata/link setup supporting at most 1 > device per link. This results in ata_find_dev() always returning NULL > except for the first device with device number 0. > > Fix this by rewriting ata_find_dev() to ignore the device number for > non-pmp attached devices with a link with at most 1 device. For these, > device number 0 is always used to return the correct ata_device struct > of the port link. This change excludes IDE master/slave setups (maximum > number of devices per link is 2) and port-multiplier attached devices. > > Reported-by: Xingui Yang <yangxingui@huawei.com> > Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 7bb12deab70c..3ba9cb258394 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc) > > static struct ata_device *ata_find_dev(struct ata_port *ap, int devno) > { > - if (!sata_pmp_attached(ap)) { > - if (likely(devno >= 0 && > - devno < ata_link_max_devices(&ap->link))) > + if (unlikely(devno < 0)) > + return NULL; > + > + if (likely(!sata_pmp_attached(ap))) { > + /* > + * For the non PMP case, the maximum number of devices per link > + * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former > + * case includes libsas hosted devices which are numbered up to > + * the number of devices scanned on an HBA port, but with each > + * ata device having its own ata port and link. To accommodate > + * these, ignore devno and always use device number 0. > + */ > + switch (ata_link_max_devices(&ap->link)) { > + case 1: > + return &ap->link.device[0]; > + case 2: > + if (devno >= 2) How about ATA_MAX_DEVICES? > + return NULL; > return &ap->link.device[devno]; > - } else { > - if (likely(devno >= 0 && > - devno < ap->nr_pmp_links)) > - return &ap->pmp_link[devno].device[0]; > + default: > + return NULL; > + } > } > > + if (devno < ap->nr_pmp_links) > + return &ap->pmp_link[devno].device[0]; > + > return NULL; > } This looks ok to me, since we have a big comment about what we're doing. I did send another suggestion, so I'll leave it to you. BTW, I think that we can follow-up to this and remove the add ata_device arg that we added to sas_change_queue_depth() Thanks, John
On 5/22/23 20:48, John Garry wrote: > On 22/05/2023 12:27, Damien Le Moal wrote: > > Hi Damien, > > Our mails just crossed... > >> For non-pmp attached devices managed directly by libata, the device >> number is always 0 or 1 and lower to the maximum number of devices >> returned by ata_link_max_devices(). However, for libsas managed devices, >> devices are numbered up to the number of device scanned on an HBA port, > > It's not really clear to me which number you mean. For libsas and lib > ata, ata_device->devno is configured the same, it's just that the sdev devno used in ata_find_dev() is scsidev->id, which is always 0 for a non-pmp SATA drive (and can be 1 for an IDE slave drive). But with libsas, that number is 0, 1, 2, 3... numbering the devices found on the HBA port. Hence ata_find_dev() return NULL always because the ata_link_max_devices() is always 1 for any libsas ata device as they all have their own ata_port & ata_link. > per ata_device does not have the same numbering scheme for libsas. For > libsas - or scsi_transport_sas, to be more exact - the sdev id is per shost. Yes. So the sdev->id goes beyond 0 and cannot be used as the devno for the devices as they each have their own port & link. > >> while each device has a regular ata/link setup supporting at most 1 >> device per link. This results in ata_find_dev() always returning NULL >> except for the first device with device number 0. >> >> Fix this by rewriting ata_find_dev() to ignore the device number for >> non-pmp attached devices with a link with at most 1 device. For these, >> device number 0 is always used to return the correct ata_device struct >> of the port link. This change excludes IDE master/slave setups (maximum >> number of devices per link is 2) and port-multiplier attached devices. >> >> Reported-by: Xingui Yang <yangxingui@huawei.com> >> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- >> 1 file changed, 24 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 7bb12deab70c..3ba9cb258394 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc) >> >> static struct ata_device *ata_find_dev(struct ata_port *ap, int devno) >> { >> - if (!sata_pmp_attached(ap)) { >> - if (likely(devno >= 0 && >> - devno < ata_link_max_devices(&ap->link))) >> + if (unlikely(devno < 0)) >> + return NULL; >> + >> + if (likely(!sata_pmp_attached(ap))) { >> + /* >> + * For the non PMP case, the maximum number of devices per link >> + * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former >> + * case includes libsas hosted devices which are numbered up to >> + * the number of devices scanned on an HBA port, but with each >> + * ata device having its own ata port and link. To accommodate >> + * these, ignore devno and always use device number 0. >> + */ >> + switch (ata_link_max_devices(&ap->link)) { >> + case 1: >> + return &ap->link.device[0]; >> + case 2: >> + if (devno >= 2) > > How about ATA_MAX_DEVICES? Indeed. That would be nicer. And ata_link_max_devices() could use that as well. > >> + return NULL; >> return &ap->link.device[devno]; >> - } else { >> - if (likely(devno >= 0 && >> - devno < ap->nr_pmp_links)) >> - return &ap->pmp_link[devno].device[0]; >> + default: >> + return NULL; >> + } >> } >> >> + if (devno < ap->nr_pmp_links) >> + return &ap->pmp_link[devno].device[0]; >> + >> return NULL; >> } > > This looks ok to me, since we have a big comment about what we're doing. I am not super happy about the wording of the comment. Suggestions welcome :) I will at least reword to mention the counting of devices per shost. > I did send another suggestion, so I'll leave it to you. I kind of like the loop as it does not need a devno but it also implies that we do have a scsi dev already while ata_find_dev() currently does not assume that. Given that this function is also used in ata_scsi_user_scan() where we do not have a sdev, it would not work for all cases. > > BTW, I think that we can follow-up to this and remove the add ata_device > arg that we added to sas_change_queue_depth() Yes. I will clean that up after sending this fix for this cycle. The cleanup will be for 6.5. > > Thanks, > John >
On 2023/5/22 19:27, Damien Le Moal wrote: > For non-pmp attached devices managed directly by libata, the device > number is always 0 or 1 and lower to the maximum number of devices > returned by ata_link_max_devices(). However, for libsas managed devices, > devices are numbered up to the number of device scanned on an HBA port, > while each device has a regular ata/link setup supporting at most 1 > device per link. This results in ata_find_dev() always returning NULL > except for the first device with device number 0. > > Fix this by rewriting ata_find_dev() to ignore the device number for > non-pmp attached devices with a link with at most 1 device. For these, > device number 0 is always used to return the correct ata_device struct > of the port link. This change excludes IDE master/slave setups (maximum > number of devices per link is 2) and port-multiplier attached devices. > > Reported-by: Xingui Yang <yangxingui@huawei.com> > Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 7bb12deab70c..3ba9cb258394 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc) > > static struct ata_device *ata_find_dev(struct ata_port *ap, int devno) > { > - if (!sata_pmp_attached(ap)) { > - if (likely(devno >= 0 && > - devno < ata_link_max_devices(&ap->link))) > + if (unlikely(devno < 0)) > + return NULL; devno is either scsidev->id or scsidev->channel, which will never < 0. Actually it is unsigned int. So I doubt if we need this check here. > + > + if (likely(!sata_pmp_attached(ap))) { > + /* > + * For the non PMP case, the maximum number of devices per link > + * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former > + * case includes libsas hosted devices which are numbered up to > + * the number of devices scanned on an HBA port, but with each > + * ata device having its own ata port and link. To accommodate > + * these, ignore devno and always use device number 0. > + */ > + switch (ata_link_max_devices(&ap->link)) { > + case 1: > + return &ap->link.device[0]; > + case 2: > + if (devno >= 2) > + return NULL; > return &ap->link.device[devno]; > - } else { > - if (likely(devno >= 0 && > - devno < ap->nr_pmp_links)) > - return &ap->pmp_link[devno].device[0]; > + default: > + return NULL; > + } > } > > + if (devno < ap->nr_pmp_links) > + return &ap->pmp_link[devno].device[0]; > + > return NULL; > } This change looks good to me. Thanks, Jason
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 7bb12deab70c..3ba9cb258394 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc) static struct ata_device *ata_find_dev(struct ata_port *ap, int devno) { - if (!sata_pmp_attached(ap)) { - if (likely(devno >= 0 && - devno < ata_link_max_devices(&ap->link))) + if (unlikely(devno < 0)) + return NULL; + + if (likely(!sata_pmp_attached(ap))) { + /* + * For the non PMP case, the maximum number of devices per link + * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former + * case includes libsas hosted devices which are numbered up to + * the number of devices scanned on an HBA port, but with each + * ata device having its own ata port and link. To accommodate + * these, ignore devno and always use device number 0. + */ + switch (ata_link_max_devices(&ap->link)) { + case 1: + return &ap->link.device[0]; + case 2: + if (devno >= 2) + return NULL; return &ap->link.device[devno]; - } else { - if (likely(devno >= 0 && - devno < ap->nr_pmp_links)) - return &ap->pmp_link[devno].device[0]; + default: + return NULL; + } } + if (devno < ap->nr_pmp_links) + return &ap->pmp_link[devno].device[0]; + return NULL; }
For non-pmp attached devices managed directly by libata, the device number is always 0 or 1 and lower to the maximum number of devices returned by ata_link_max_devices(). However, for libsas managed devices, devices are numbered up to the number of device scanned on an HBA port, while each device has a regular ata/link setup supporting at most 1 device per link. This results in ata_find_dev() always returning NULL except for the first device with device number 0. Fix this by rewriting ata_find_dev() to ignore the device number for non-pmp attached devices with a link with at most 1 device. For these, device number 0 is always used to return the correct ata_device struct of the port link. This change excludes IDE master/slave setups (maximum number of devices per link is 2) and port-multiplier attached devices. Reported-by: Xingui Yang <yangxingui@huawei.com> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)