| Message ID | 20230605013212.573489-4-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Cleanups and improvements | expand |
On 6/5/23 03:32, Damien Le Moal wrote: > In ata_scsi_dev_config(), instead of hardconing the test to check if > an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use > ata_ncq_supported(). > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 8ce90284eb34..22e2e9ab6b60 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1122,7 +1122,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) > if (dev->flags & ATA_DFLAG_AN) > set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); > > - if (dev->flags & ATA_DFLAG_NCQ) > + if (ata_ncq_supported(dev)) > depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); > depth = min(ATA_MAX_QUEUE, depth); > scsi_change_queue_depth(sdev, depth); Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO, ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about). Can we please move them into some more descriptive, ie which flags are for the drive capabilities (ie _can_ the drive do NCQ) and the current current drive status (ie _does_ the drive do NCQ)? As it stands it's quite confusing. But probably not a problem with this patch, so: Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 6/5/23 17:04, Hannes Reinecke wrote: > On 6/5/23 03:32, Damien Le Moal wrote: >> In ata_scsi_dev_config(), instead of hardconing the test to check if >> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use >> ata_ncq_supported(). >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> drivers/ata/libata-scsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 8ce90284eb34..22e2e9ab6b60 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -1122,7 +1122,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) >> if (dev->flags & ATA_DFLAG_AN) >> set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); >> >> - if (dev->flags & ATA_DFLAG_NCQ) >> + if (ata_ncq_supported(dev)) >> depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); >> depth = min(ATA_MAX_QUEUE, depth); >> scsi_change_queue_depth(sdev, depth); > > Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO, > ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about). > Can we please move them into some more descriptive, ie which flags > are for the drive capabilities (ie _can_ the drive do NCQ) and > the current current drive status (ie _does_ the drive do NCQ)? > As it stands it's quite confusing. In include/linux/libata.h, we have: ATA_DFLAG_NCQ = (1 << 3), /* device supports NCQ */ ATA_DFLAG_PIO = (1 << 13), /* device limited to PIO mode */ ATA_DFLAG_NCQ_OFF = (1 << 14), /* device limited to non-NCQ mode */ So there are some description. Not enough ? > > But probably not a problem with this patch, so: > > Reviewed-by: Hannes Reinecke <hare@suse.de> > > Cheers, > > Hannes
On 6/5/23 11:37, Damien Le Moal wrote: > On 6/5/23 17:04, Hannes Reinecke wrote: >> On 6/5/23 03:32, Damien Le Moal wrote: >>> In ata_scsi_dev_config(), instead of hardconing the test to check if >>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use >>> ata_ncq_supported(). >>> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>> --- >>> drivers/ata/libata-scsi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>> index 8ce90284eb34..22e2e9ab6b60 100644 >>> --- a/drivers/ata/libata-scsi.c >>> +++ b/drivers/ata/libata-scsi.c >>> @@ -1122,7 +1122,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) >>> if (dev->flags & ATA_DFLAG_AN) >>> set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); >>> >>> - if (dev->flags & ATA_DFLAG_NCQ) >>> + if (ata_ncq_supported(dev)) >>> depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); >>> depth = min(ATA_MAX_QUEUE, depth); >>> scsi_change_queue_depth(sdev, depth); >> >> Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO, >> ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about). >> Can we please move them into some more descriptive, ie which flags >> are for the drive capabilities (ie _can_ the drive do NCQ) and >> the current current drive status (ie _does_ the drive do NCQ)? >> As it stands it's quite confusing. > > In include/linux/libata.h, we have: > > ATA_DFLAG_NCQ = (1 << 3), /* device supports NCQ */ > ATA_DFLAG_PIO = (1 << 13), /* device limited to PIO mode */ > ATA_DFLAG_NCQ_OFF = (1 << 14), /* device limited to non-NCQ mode */ > > So there are some description. Not enough ? > Well. Guess my point is that ATA_DFLAG_PIO is a device status (which might change during runtime), whereas ATA_DFLAG_NCQ is a device setting (either it does or does not support NCQ). And ATA_DFLAG_NCQ_OFF is again a device status (device supports NCQ, but it's disabled for whatever reason). I'd rather have a more descriptive naming like ATA_DFLAG_NCQ_SUPPORTED to clearly indicate that this flag is not about what the driver _currently_ is using (as opposed to ATA_DFLAG_PIO), but rather a static device configuration which won't change whatever I do. Cheers, Hannes
On 6/5/23 4:32 AM, Damien Le Moal wrote: > In ata_scsi_dev_config(), instead of hardconing the test to check if Again, hardcoding? > an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use > ata_ncq_supported(). > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> [...] MBR, Sergey
On 6/5/23 18:47, Sergei Shtylyov wrote: > On 6/5/23 4:32 AM, Damien Le Moal wrote: > >> In ata_scsi_dev_config(), instead of hardconing the test to check if > > Again, hardcoding? Yes... Square fingers today :) > >> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use >> ata_ncq_supported(). >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > [...] > > MBR, Sergey
On 6/5/23 18:46, Hannes Reinecke wrote: > On 6/5/23 11:37, Damien Le Moal wrote: >> On 6/5/23 17:04, Hannes Reinecke wrote: >>> On 6/5/23 03:32, Damien Le Moal wrote: >>>> In ata_scsi_dev_config(), instead of hardconing the test to check if >>>> an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use >>>> ata_ncq_supported(). >>>> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>> --- >>>> drivers/ata/libata-scsi.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>>> index 8ce90284eb34..22e2e9ab6b60 100644 >>>> --- a/drivers/ata/libata-scsi.c >>>> +++ b/drivers/ata/libata-scsi.c >>>> @@ -1122,7 +1122,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) >>>> if (dev->flags & ATA_DFLAG_AN) >>>> set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); >>>> >>>> - if (dev->flags & ATA_DFLAG_NCQ) >>>> + if (ata_ncq_supported(dev)) >>>> depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); >>>> depth = min(ATA_MAX_QUEUE, depth); >>>> scsi_change_queue_depth(sdev, depth); >>> >>> Argh. ATA NCQ flags. We have ATA_DFLAG_NCQ, ATA_DFLAG_PIO, >>> ATA_DFLAG_NCQ_OFF (and maybe even more which I forgot about). >>> Can we please move them into some more descriptive, ie which flags >>> are for the drive capabilities (ie _can_ the drive do NCQ) and >>> the current current drive status (ie _does_ the drive do NCQ)? >>> As it stands it's quite confusing. >> >> In include/linux/libata.h, we have: >> >> ATA_DFLAG_NCQ = (1 << 3), /* device supports NCQ */ >> ATA_DFLAG_PIO = (1 << 13), /* device limited to PIO mode */ >> ATA_DFLAG_NCQ_OFF = (1 << 14), /* device limited to non-NCQ mode */ >> >> So there are some description. Not enough ? >> > Well. Guess my point is that ATA_DFLAG_PIO is a device status (which > might change during runtime), whereas ATA_DFLAG_NCQ is a device setting > (either it does or does not support NCQ). > And ATA_DFLAG_NCQ_OFF is again a device status (device supports NCQ, but > it's disabled for whatever reason). > I'd rather have a more descriptive naming like > ATA_DFLAG_NCQ_SUPPORTED > to clearly indicate that this flag is not about what the driver > _currently_ is using (as opposed to ATA_DFLAG_PIO), but rather a static > device configuration which won't change whatever I do. Yes, agree. There is also some funky logic going on with all this: ATA_DFLAG_PIO meaning "PIO supported" should always be set for NCQ drives, because NCQ drives necessarily can do DMA and so they can do PIO as well... So patterns like: dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ | ATA_DFLAG_NCQ_OFF)) == ATA_DFLAG_NCQ To test if NCQ is supported does not really make any sense... Example: ata_scsi_security_inout_xlat() has: bool dma = !(qc->dev->flags & ATA_DFLAG_PIO); Sure... That works, but that is not technically super correct has a DMA capable drive can do PIO as well. I think we are dealing with ata history here as everything was "PIO" at the beginning of ata times, so that flag did not really needed to exist and was added later to differentiate with NCQ case. Splitting the gigantic enum in include/linux/libata.h into manageable pieces (e.g. "enum ata_device_flags { ... };") with some renaming and better kdocs would help clarifies things. Will try to work on that.
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 8ce90284eb34..22e2e9ab6b60 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1122,7 +1122,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) if (dev->flags & ATA_DFLAG_AN) set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); - if (dev->flags & ATA_DFLAG_NCQ) + if (ata_ncq_supported(dev)) depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); depth = min(ATA_MAX_QUEUE, depth); scsi_change_queue_depth(sdev, depth);
In ata_scsi_dev_config(), instead of hardconing the test to check if an ATA device supports NCQ by looking at the ATA_DFLAG_NCQ flag, use ata_ncq_supported(). Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)