| Message ID | 20240726031954.566882-5-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Some cleanup, renaming and horkage improvements | expand |
On Fri, Jul 26, 2024 at 12:19:47PM +0900, Damien Le Moal wrote: > Introduce the function ata_dev_print_quirks() to print the quirk flags > that will be applied to a scanned device. This new function is called > from ata_dev_quirks() when a match on a device model or device model > and revision is found for a device in the __ata_dev_quirks array. > > To implement this function, the ATA_QUIRK_ flags are redefined using > the new enum ata_quirk which defines the bit shift for each quirk > flag. The array of strings ata_quirk_names is used to define the name > of each flag, which are printed by ata_dev_print_quirks(). > > Example output for a device listed in the __ata_dev_quirks array and > which has the ATA_QUIRK_DISABLE flag applied: > > [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable > [10193.469195] ata1.00: unsupported device, disabling > [10193.481564] ata1.00: disable device > > enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the > last quirk flag defined. This value is used in ata_dev_quirks() to add a > build time check that all quirk flags fit within the unsigned int > (32-bits) quirks field of struct ata_device. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-core.c | 76 +++++++++++++++++++++++++-- > include/linux/libata.h | 106 ++++++++++++++++++++++++++------------ > 2 files changed, 143 insertions(+), 39 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 19b041bd7588..fc9fcfda42b8 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -3988,6 +3988,69 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class, > return rc; > } > > +static const char * const ata_quirk_names[] = { > + [__ATA_QUIRK_DIAGNOSTIC] = "diagnostic", > + [__ATA_QUIRK_NODMA] = "nodma", > + [__ATA_QUIRK_NONCQ] = "noncq", > + [__ATA_QUIRK_MAX_SEC_128] = "maxsec128", > + [__ATA_QUIRK_BROKEN_HPA] = "brokenhpa", > + [__ATA_QUIRK_DISABLE] = "disable", > + [__ATA_QUIRK_HPA_SIZE] = "hpasize", > + [__ATA_QUIRK_IVB] = "ivb", > + [__ATA_QUIRK_STUCK_ERR] = "stuckerr", > + [__ATA_QUIRK_BRIDGE_OK] = "bridgeok", > + [__ATA_QUIRK_ATAPI_MOD16_DMA] = "atapimod16dma", > + [__ATA_QUIRK_FIRMWARE_WARN] = "firmwarewarn", > + [__ATA_QUIRK_1_5_GBPS] = "1.5gbps", > + [__ATA_QUIRK_NOSETXFER] = "nosetxfer", > + [__ATA_QUIRK_BROKEN_FPDMA_AA] = "brokenfpdmaaa", > + [__ATA_QUIRK_DUMP_ID] = "dumpid", > + [__ATA_QUIRK_MAX_SEC_LBA48] = "maxseclba48", > + [__ATA_QUIRK_ATAPI_DMADIR] = "atapidmadir", > + [__ATA_QUIRK_NO_NCQ_TRIM] = "noncqtrim", > + [__ATA_QUIRK_NOLPM] = "nolpm", > + [__ATA_QUIRK_WD_BROKEN_LPM] = "wdbrokenlpm", > + [__ATA_QUIRK_ZERO_AFTER_TRIM] = "zeroaftertrim", > + [__ATA_QUIRK_NO_DMA_LOG] = "nodmalog", > + [__ATA_QUIRK_NOTRIM] = "notrim", > + [__ATA_QUIRK_MAX_SEC_1024] = "maxsec1024", > + [__ATA_QUIRK_MAX_TRIM_128M] = "maxtrim128m", > + [__ATA_QUIRK_NO_NCQ_ON_ATI] = "noncqonati", > + [__ATA_QUIRK_NO_ID_DEV_LOG] = "noiddevlog", > + [__ATA_QUIRK_NO_LOG_DIR] = "nologdir", > + [__ATA_QUIRK_NO_FUA] = "nofua", > +}; > + > +static void ata_dev_print_quirks(const struct ata_device *dev, > + const char *model, const char *rev, > + unsigned int quirks) > +{ > + int n = 0, i; > + size_t sz; > + char *str; > + > + if (!quirks) > + return; > + > + sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; > + str = kmalloc(sz, GFP_KERNEL); > + if (!str) > + return; > + > + n = snprintf(str, sz, "Model '%s', rev '%s', applying quirks:", > + model, rev); > + > + for (i = 0; i < ARRAY_SIZE(ata_quirk_names); i++) { > + if (quirks & (1U << i)) > + n += snprintf(str + n, sz - n, > + " %s", ata_quirk_names[i]); > + } > + > + ata_dev_warn(dev, "%s\n", str); > + > + kfree(str); > +} > + > struct ata_dev_quirks_entry { > const char *model_num; > const char *model_rev; > @@ -4273,15 +4336,18 @@ static unsigned int ata_dev_quirks(const struct ata_device *dev) > unsigned char model_rev[ATA_ID_FW_REV_LEN + 1]; > const struct ata_dev_quirks_entry *ad = __ata_dev_quirks; > > + /* dev->quirks is an unsigned int. */ > + BUILD_BUG_ON(__ATA_QUIRK_MAX > 32); > + > ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num)); > ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev)); > > while (ad->model_num) { > - if (glob_match(ad->model_num, model_num)) { > - if (ad->model_rev == NULL) > - return ad->quirks; > - if (glob_match(ad->model_rev, model_rev)) > - return ad->quirks; > + if (glob_match(ad->model_num, model_num) && > + (!ad->model_rev || glob_match(ad->model_rev, model_rev))) { > + ata_dev_print_quirks(dev, model_num, model_rev, > + ad->quirks); > + return ad->quirks; > } > ad++; > } > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 05dd7038ab30..d598ef690e50 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -55,6 +55,46 @@ > /* defines only for the constants which don't work well as enums */ > #define ATA_TAG_POISON 0xfafbfcfdU > > +/* > + * Quirk flags bits. > + * ata_device->quirks is an unsigned int, so __ATA_QUIRK_MAX must not exceed 32. > + */ > +enum ata_quirks { > + __ATA_QUIRK_DIAGNOSTIC, /* Failed boot diag */ > + __ATA_QUIRK_NODMA, /* DMA problems */ > + __ATA_QUIRK_NONCQ, /* Don't use NCQ */ > + __ATA_QUIRK_MAX_SEC_128, /* Limit max sects to 128 */ > + __ATA_QUIRK_BROKEN_HPA, /* Broken HPA */ > + __ATA_QUIRK_DISABLE, /* Disable it */ > + __ATA_QUIRK_HPA_SIZE, /* Native size off by one */ > + __ATA_QUIRK_IVB, /* cbl det validity bit bugs */ > + __ATA_QUIRK_STUCK_ERR, /* Stuck ERR on next PACKET */ > + __ATA_QUIRK_BRIDGE_OK, /* No bridge limits */ > + __ATA_QUIRK_ATAPI_MOD16_DMA, /* Use ATAPI DMA for commands that */ > + /* are not a multiple of 16 bytes */ > + __ATA_QUIRK_FIRMWARE_WARN, /* Firmware update warning */ > + __ATA_QUIRK_1_5_GBPS, /* Force 1.5 Gbps */ > + __ATA_QUIRK_NOSETXFER, /* Skip SETXFER, SATA only */ > + __ATA_QUIRK_BROKEN_FPDMA_AA, /* Skip AA */ > + __ATA_QUIRK_DUMP_ID, /* Dump IDENTIFY data */ > + __ATA_QUIRK_MAX_SEC_LBA48, /* Set max sects to 65535 */ > + __ATA_QUIRK_ATAPI_DMADIR, /* Device requires dmadir */ > + __ATA_QUIRK_NO_NCQ_TRIM, /* Do not use queued TRIM */ > + __ATA_QUIRK_NOLPM, /* Do not use LPM */ > + __ATA_QUIRK_WD_BROKEN_LPM, /* Some WDs have broken LPM */ > + __ATA_QUIRK_ZERO_AFTER_TRIM, /* Guarantees zero after trim */ > + __ATA_QUIRK_NO_DMA_LOG, /* Do not use DMA for log read */ > + __ATA_QUIRK_NOTRIM, /* Do not use TRIM */ > + __ATA_QUIRK_MAX_SEC_1024, /* Limit max sects to 1024 */ > + __ATA_QUIRK_MAX_TRIM_128M, /* Limit max trim size to 128M */ > + __ATA_QUIRK_NO_NCQ_ON_ATI, /* Disable NCQ on ATI chipset */ > + __ATA_QUIRK_NO_ID_DEV_LOG, /* Identify device log missing */ > + __ATA_QUIRK_NO_LOG_DIR, /* Do not read log directory */ > + __ATA_QUIRK_NO_FUA, /* Do not use FUA */ > + > + __ATA_QUIRK_MAX, > +}; > + > enum { > /* various global constants */ > LIBATA_MAX_PRD = ATA_MAX_PRD / 2, > @@ -366,40 +406,38 @@ enum { > * Quirk flags: may be set by libata or controller drivers on drives. > * Some quirks may be drive/controller pair dependent. > */ > - ATA_QUIRK_DIAGNOSTIC = (1 << 0), /* Failed boot diag */ > - ATA_QUIRK_NODMA = (1 << 1), /* DMA problems */ > - ATA_QUIRK_NONCQ = (1 << 2), /* Do not use NCQ */ > - ATA_QUIRK_MAX_SEC_128 = (1 << 3), /* Limit max sects to 128 */ > - ATA_QUIRK_BROKEN_HPA = (1 << 4), /* Broken HPA */ > - ATA_QUIRK_DISABLE = (1 << 5), /* Disable it */ > - ATA_QUIRK_HPA_SIZE = (1 << 6), /* Native size off by one */ > - ATA_QUIRK_IVB = (1 << 8), /* CBL det validity bit bugs */ > - ATA_QUIRK_STUCK_ERR = (1 << 9), /* Stuck ERR on next PACKET */ > - ATA_QUIRK_BRIDGE_OK = (1 << 10), /* No bridge limits */ > - ATA_QUIRK_ATAPI_MOD16_DMA = (1 << 11), /* Use ATAPI DMA for commands */ > - /* not multiple of 16 bytes */ > - ATA_QUIRK_FIRMWARE_WARN = (1 << 12), /* Firmware update warning */ > - ATA_QUIRK_1_5_GBPS = (1 << 13), /* Force 1.5 Gbps */ > - ATA_QUIRK_NOSETXFER = (1 << 14), /* Skip SETXFER, SATA only */ > - ATA_QUIRK_BROKEN_FPDMA_AA = (1 << 15), /* Skip AA */ > - ATA_QUIRK_DUMP_ID = (1 << 16), /* Dump IDENTIFY data */ > - ATA_QUIRK_MAX_SEC_LBA48 = (1 << 17), /* Set max sects to 65535 */ > - ATA_QUIRK_ATAPI_DMADIR = (1 << 18), /* Device requires dmadir */ > - ATA_QUIRK_NO_NCQ_TRIM = (1 << 19), /* Do not use queued TRIM */ > - ATA_QUIRK_NOLPM = (1 << 20), /* Do not use LPM */ > - ATA_QUIRK_WD_BROKEN_LPM = (1 << 21), /* Some WDs have broken LPM */ > - ATA_QUIRK_ZERO_AFTER_TRIM = (1 << 22), /* Guarantees zero after trim */ > - ATA_QUIRK_NO_DMA_LOG = (1 << 23), /* Do not use DMA for log read */ > - ATA_QUIRK_NOTRIM = (1 << 24), /* Do not use TRIM */ > - ATA_QUIRK_MAX_SEC_1024 = (1 << 25), /* Limit max sects to 1024 */ > - ATA_QUIRK_MAX_TRIM_128M = (1 << 26), /* Limit max trim size to 128M */ > - ATA_QUIRK_NO_NCQ_ON_ATI = (1 << 27), /* Disable NCQ on ATI chipset */ > - ATA_QUIRK_NO_ID_DEV_LOG = (1 << 28), /* Identify device log missing */ > - ATA_QUIRK_NO_LOG_DIR = (1 << 29), /* Do not read log directory */ > - ATA_QUIRK_NO_FUA = (1 << 30), /* Do not use FUA */ > - > - /* DMA mask for user DMA control: User visible values; DO NOT > - renumber */ > + ATA_QUIRK_DIAGNOSTIC = (1U << __ATA_QUIRK_DIAGNOSTIC), > + ATA_QUIRK_NODMA = (1U << __ATA_QUIRK_NODMA), > + ATA_QUIRK_NONCQ = (1U << __ATA_QUIRK_NONCQ), > + ATA_QUIRK_MAX_SEC_128 = (1U << __ATA_QUIRK_MAX_SEC_128), > + ATA_QUIRK_BROKEN_HPA = (1U << __ATA_QUIRK_BROKEN_HPA), > + ATA_QUIRK_DISABLE = (1U << __ATA_QUIRK_DISABLE), > + ATA_QUIRK_HPA_SIZE = (1U << __ATA_QUIRK_HPA_SIZE), > + ATA_QUIRK_IVB = (1U << __ATA_QUIRK_IVB), > + ATA_QUIRK_STUCK_ERR = (1U << __ATA_QUIRK_STUCK_ERR), > + ATA_QUIRK_BRIDGE_OK = (1U << __ATA_QUIRK_BRIDGE_OK), > + ATA_QUIRK_ATAPI_MOD16_DMA = (1U << __ATA_QUIRK_ATAPI_MOD16_DMA), > + ATA_QUIRK_FIRMWARE_WARN = (1U << __ATA_QUIRK_FIRMWARE_WARN), > + ATA_QUIRK_1_5_GBPS = (1U << __ATA_QUIRK_1_5_GBPS), > + ATA_QUIRK_NOSETXFER = (1U << __ATA_QUIRK_NOSETXFER), > + ATA_QUIRK_BROKEN_FPDMA_AA = (1U << __ATA_QUIRK_BROKEN_FPDMA_AA), > + ATA_QUIRK_DUMP_ID = (1U << __ATA_QUIRK_DUMP_ID), > + ATA_QUIRK_MAX_SEC_LBA48 = (1U << __ATA_QUIRK_MAX_SEC_LBA48), > + ATA_QUIRK_ATAPI_DMADIR = (1U << __ATA_QUIRK_ATAPI_DMADIR), > + ATA_QUIRK_NO_NCQ_TRIM = (1U << __ATA_QUIRK_NO_NCQ_TRIM), > + ATA_QUIRK_NOLPM = (1U << __ATA_QUIRK_NOLPM), > + ATA_QUIRK_WD_BROKEN_LPM = (1U << __ATA_QUIRK_WD_BROKEN_LPM), > + ATA_QUIRK_ZERO_AFTER_TRIM = (1U << __ATA_QUIRK_ZERO_AFTER_TRIM), > + ATA_QUIRK_NO_DMA_LOG = (1U << __ATA_QUIRK_NO_DMA_LOG), > + ATA_QUIRK_NOTRIM = (1U << __ATA_QUIRK_NOTRIM), > + ATA_QUIRK_MAX_SEC_1024 = (1U << __ATA_QUIRK_MAX_SEC_1024), > + ATA_QUIRK_MAX_TRIM_128M = (1U << __ATA_QUIRK_MAX_TRIM_128M), > + ATA_QUIRK_NO_NCQ_ON_ATI = (1U << __ATA_QUIRK_NO_NCQ_ON_ATI), > + ATA_QUIRK_NO_ID_DEV_LOG = (1U << __ATA_QUIRK_NO_ID_DEV_LOG), > + ATA_QUIRK_NO_LOG_DIR = (1U << __ATA_QUIRK_NO_LOG_DIR), > + ATA_QUIRK_NO_FUA = (1U << __ATA_QUIRK_NO_FUA), Personally, I would prefer if these used the BIT() macro. But since BIT() does the shift with 1UL, that might require that this patch (and the previous patch in the series) changes the quirk data type to consistenly be "unsigned long" everywhere, instead of changing the quirk data type to consistently be "unsigned int" everywhere. We could still keep the: BUILD_BUG_ON(__ATA_QUIRK_MAX > 32); Since unsigned long is 32-bits on 32-bit platforms. FWIW, on nvme, quirks is defined as "unsigned long" in drivers/nvme/host/nvme.h But since this is personal preference, I would also be fine if you decide to keep this patch as it is, thus: Reviewed-by: Niklas Cassel <cassel@kernel.org>
Hi Damien, On Fri, 26 Jul 2024, Damien Le Moal wrote: > Introduce the function ata_dev_print_quirks() to print the quirk flags > that will be applied to a scanned device. This new function is called > from ata_dev_quirks() when a match on a device model or device model > and revision is found for a device in the __ata_dev_quirks array. > > To implement this function, the ATA_QUIRK_ flags are redefined using > the new enum ata_quirk which defines the bit shift for each quirk > flag. The array of strings ata_quirk_names is used to define the name > of each flag, which are printed by ata_dev_print_quirks(). > > Example output for a device listed in the __ata_dev_quirks array and > which has the ATA_QUIRK_DISABLE flag applied: > > [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable > [10193.469195] ata1.00: unsupported device, disabling > [10193.481564] ata1.00: disable device > > enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the > last quirk flag defined. This value is used in ata_dev_quirks() to add a > build time check that all quirk flags fit within the unsigned int > (32-bits) quirks field of struct ata_device. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > Reviewed-by: Igor Pylypiv <ipylypiv@google.com> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: libata: Print quirks applied to devices") in libata/for-next. > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -4273,15 +4336,18 @@ static unsigned int ata_dev_quirks(const struct ata_device *dev) > unsigned char model_rev[ATA_ID_FW_REV_LEN + 1]; > const struct ata_dev_quirks_entry *ad = __ata_dev_quirks; > > + /* dev->quirks is an unsigned int. */ > + BUILD_BUG_ON(__ATA_QUIRK_MAX > 32); > + > ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num)); > ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev)); > > while (ad->model_num) { > - if (glob_match(ad->model_num, model_num)) { > - if (ad->model_rev == NULL) > - return ad->quirks; > - if (glob_match(ad->model_rev, model_rev)) > - return ad->quirks; > + if (glob_match(ad->model_num, model_num) && > + (!ad->model_rev || glob_match(ad->model_rev, model_rev))) { > + ata_dev_print_quirks(dev, model_num, model_rev, > + ad->quirks); > + return ad->quirks; > } > ad++; > } During boot-up on Salvator-XS (using rcar-sata), the quirk info is printed not once, but four times. Is that intentional? ata1: link resume succeeded after 1 retries +rcar-du feb00000.display: [drm] fb0: rcar-dudrmfb frame buffer device input: keys as /devices/platform/keys/input/input0 ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq ata1.00: configured for UDMA/133 scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) sd 0:0:0:0: [sda] Write Protect is off sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes sda: sda1 sd 0:0:0:0: [sda] Attached SCSI disk During resume from s2idle or s2ram, the same info is printed again, fourfold: ata1: link resume succeeded after 1 retries ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq ata1.00: configured for UDMA/133 ata1.00: Entering active power mode Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 7/30/24 19:09, Geert Uytterhoeven wrote: > Hi Damien, > > On Fri, 26 Jul 2024, Damien Le Moal wrote: >> Introduce the function ata_dev_print_quirks() to print the quirk flags >> that will be applied to a scanned device. This new function is called >> from ata_dev_quirks() when a match on a device model or device model >> and revision is found for a device in the __ata_dev_quirks array. >> >> To implement this function, the ATA_QUIRK_ flags are redefined using >> the new enum ata_quirk which defines the bit shift for each quirk >> flag. The array of strings ata_quirk_names is used to define the name >> of each flag, which are printed by ata_dev_print_quirks(). >> >> Example output for a device listed in the __ata_dev_quirks array and >> which has the ATA_QUIRK_DISABLE flag applied: >> >> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) >> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable >> [10193.469195] ata1.00: unsupported device, disabling >> [10193.481564] ata1.00: disable device >> >> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the >> last quirk flag defined. This value is used in ata_dev_quirks() to add a >> build time check that all quirk flags fit within the unsigned int >> (32-bits) quirks field of struct ata_device. >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > > Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: > libata: Print quirks applied to devices") in libata/for-next. > >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -4273,15 +4336,18 @@ static unsigned int ata_dev_quirks(const struct ata_device *dev) >> unsigned char model_rev[ATA_ID_FW_REV_LEN + 1]; >> const struct ata_dev_quirks_entry *ad = __ata_dev_quirks; >> >> + /* dev->quirks is an unsigned int. */ >> + BUILD_BUG_ON(__ATA_QUIRK_MAX > 32); >> + >> ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num)); >> ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev)); >> >> while (ad->model_num) { >> - if (glob_match(ad->model_num, model_num)) { >> - if (ad->model_rev == NULL) >> - return ad->quirks; >> - if (glob_match(ad->model_rev, model_rev)) >> - return ad->quirks; >> + if (glob_match(ad->model_num, model_num) && >> + (!ad->model_rev || glob_match(ad->model_rev, model_rev))) { >> + ata_dev_print_quirks(dev, model_num, model_rev, >> + ad->quirks); >> + return ad->quirks; >> } >> ad++; >> } > > During boot-up on Salvator-XS (using rcar-sata), the quirk info is > printed not once, but four times. Is that intentional? Not at all. I tested on x86 with AHCI and see this message only once. So it could be that different drivers may need some tweaks to avoid this spamming. Though it is strange that the initialization or resume path takes this path 4 times, meaning that the quirks are applied 4 times. Need to look into that. What is the driver for rcar-sata ? Compatible string for it would be fine. > > ata1: link resume succeeded after 1 retries > +rcar-du feb00000.display: [drm] fb0: rcar-dudrmfb frame buffer device > input: keys as /devices/platform/keys/input/input0 > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 > ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: configured for UDMA/133 > scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 > sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) > sd 0:0:0:0: [sda] Write Protect is off > sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA > sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > sda: sda1 > sd 0:0:0:0: [sda] Attached SCSI disk > > During resume from s2idle or s2ram, the same info is printed again, > fourfold: > > ata1: link resume succeeded after 1 retries > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: configured for UDMA/133 > ata1.00: Entering active power mode > > Thanks! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Damien, On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: > On 7/30/24 19:09, Geert Uytterhoeven wrote: > > On Fri, 26 Jul 2024, Damien Le Moal wrote: > >> Introduce the function ata_dev_print_quirks() to print the quirk flags > >> that will be applied to a scanned device. This new function is called > >> from ata_dev_quirks() when a match on a device model or device model > >> and revision is found for a device in the __ata_dev_quirks array. > >> > >> To implement this function, the ATA_QUIRK_ flags are redefined using > >> the new enum ata_quirk which defines the bit shift for each quirk > >> flag. The array of strings ata_quirk_names is used to define the name > >> of each flag, which are printed by ata_dev_print_quirks(). > >> > >> Example output for a device listed in the __ata_dev_quirks array and > >> which has the ATA_QUIRK_DISABLE flag applied: > >> > >> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > >> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable > >> [10193.469195] ata1.00: unsupported device, disabling > >> [10193.481564] ata1.00: disable device > >> > >> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the > >> last quirk flag defined. This value is used in ata_dev_quirks() to add a > >> build time check that all quirk flags fit within the unsigned int > >> (32-bits) quirks field of struct ata_device. > >> > >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > > > > Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: > > libata: Print quirks applied to devices") in libata/for-next. > > > > During boot-up on Salvator-XS (using rcar-sata), the quirk info is > > printed not once, but four times. Is that intentional? > > Not at all. I tested on x86 with AHCI and see this message only once. So it > could be that different drivers may need some tweaks to avoid this spamming. > Though it is strange that the initialization or resume path takes this path 4 > times, meaning that the quirks are applied 4 times. Need to look into that. > What is the driver for rcar-sata ? Compatible string for it would be fine. drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. I added a WARN() to ata_dev_quirks() to show backtraces: Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0x74/0x12d8 ata_eh_recover+0x8d8/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0xf34/0x12d8 ata_eh_recover+0x8d8/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0x74/0x12d8 ata_dev_revalidate+0xb4/0x1b8 ata_do_set_mode+0x534/0x6bc ata_set_mode+0xc8/0x128 ata_eh_recover+0x944/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0xf34/0x12d8 ata_dev_revalidate+0xb4/0x1b8 ata_do_set_mode+0x534/0x6bc ata_set_mode+0xc8/0x128 ata_eh_recover+0x944/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 The backtraces seen during s2idle are slightly different: Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0x74/0x12d8 ata_dev_revalidate+0xb4/0x1b8 ata_eh_recover+0x7b4/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0xf34/0x12d8 ata_dev_revalidate+0xb4/0x1b8 ata_eh_recover+0x7b4/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0x74/0x12d8 ata_dev_revalidate+0xb4/0x1b8 ata_do_set_mode+0x534/0x6bc ata_set_mode+0xc8/0x128 ata_eh_recover+0x944/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Call trace: ata_dev_quirks+0x98/0x19c ata_dev_configure+0xf34/0x12d8 ata_dev_revalidate+0xb4/0x1b8 ata_do_set_mode+0x534/0x6bc ata_set_mode+0xc8/0x128 ata_eh_recover+0x944/0xd08 ata_do_eh+0x50/0xa8 ata_sff_error_handler+0xd0/0xec ata_bmdma_error_handler+0x7c/0x12c ata_scsi_port_error_handler+0xc8/0x5f8 ata_scsi_error+0x90/0xcc scsi_error_handler+0x148/0x308 kthread+0xe4/0xf4 ret_from_fork+0x10/0x20 Gr{oetje,eeting}s, Geert
On 7/31/24 16:27, Geert Uytterhoeven wrote: > Hi Damien, > > On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: >> On 7/30/24 19:09, Geert Uytterhoeven wrote: >>> On Fri, 26 Jul 2024, Damien Le Moal wrote: >>>> Introduce the function ata_dev_print_quirks() to print the quirk flags >>>> that will be applied to a scanned device. This new function is called >>>> from ata_dev_quirks() when a match on a device model or device model >>>> and revision is found for a device in the __ata_dev_quirks array. >>>> >>>> To implement this function, the ATA_QUIRK_ flags are redefined using >>>> the new enum ata_quirk which defines the bit shift for each quirk >>>> flag. The array of strings ata_quirk_names is used to define the name >>>> of each flag, which are printed by ata_dev_print_quirks(). >>>> >>>> Example output for a device listed in the __ata_dev_quirks array and >>>> which has the ATA_QUIRK_DISABLE flag applied: >>>> >>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) >>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable >>>> [10193.469195] ata1.00: unsupported device, disabling >>>> [10193.481564] ata1.00: disable device >>>> >>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the >>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a >>>> build time check that all quirk flags fit within the unsigned int >>>> (32-bits) quirks field of struct ata_device. >>>> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> >>> >>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: >>> libata: Print quirks applied to devices") in libata/for-next. >>> >>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is >>> printed not once, but four times. Is that intentional? >> >> Not at all. I tested on x86 with AHCI and see this message only once. So it >> could be that different drivers may need some tweaks to avoid this spamming. >> Though it is strange that the initialization or resume path takes this path 4 >> times, meaning that the quirks are applied 4 times. Need to look into that. >> What is the driver for rcar-sata ? Compatible string for it would be fine. > > drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. > > I added a WARN() to ata_dev_quirks() to show backtraces: > > Call trace: > ata_dev_quirks+0x98/0x19c > ata_dev_configure+0x74/0x12d8 > ata_eh_recover+0x8d8/0xd08 > ata_do_eh+0x50/0xa8 > ata_sff_error_handler+0xd0/0xec > ata_bmdma_error_handler+0x7c/0x12c > ata_scsi_port_error_handler+0xc8/0x5f8 > ata_scsi_error+0x90/0xcc > scsi_error_handler+0x148/0x308 > kthread+0xe4/0xf4 > ret_from_fork+0x10/0x20 OK. So it is ata_dev_configure() being called many times from EH. Weird. But I have not a lot of experience with the bmdma drivers. Need to look into that. In the meantime, can you try this ? diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 10d61c7523f0..24344de57428 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -76,7 +76,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev, u16 heads, u16 sectors); static unsigned int ata_dev_set_xfermode(struct ata_device *dev); static void ata_dev_xfermask(struct ata_device *dev); -static unsigned int ata_dev_quirks(const struct ata_device *dev); +static unsigned int ata_dev_quirks(struct ata_device *dev); static DEFINE_IDA(ata_ida); @@ -4079,7 +4079,7 @@ static const char * const ata_quirk_names[] = { [__ATA_QUIRK_NO_FUA] = "nofua", }; -static void ata_dev_print_quirks(const struct ata_device *dev, +static void ata_dev_print_quirks(struct ata_device *dev, const char *model, const char *rev, unsigned int quirks) { @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, size_t sz; char *str; - if (!quirks) + if (!ata_dev_print_info(dev) || !quirks) return; sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; @@ -4388,7 +4388,7 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = { { } }; -static unsigned int ata_dev_quirks(const struct ata_device *dev) +static unsigned int ata_dev_quirks(struct ata_device *dev) { unsigned char model_num[ATA_ID_PROD_LEN + 1]; unsigned char model_rev[ATA_ID_FW_REV_LEN + 1]; That should remove the multiple prints.
Hi Damien, On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@kernel.org> wrote: > On 7/31/24 16:27, Geert Uytterhoeven wrote: > > On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: > >> On 7/30/24 19:09, Geert Uytterhoeven wrote: > >>> On Fri, 26 Jul 2024, Damien Le Moal wrote: > >>>> Introduce the function ata_dev_print_quirks() to print the quirk flags > >>>> that will be applied to a scanned device. This new function is called > >>>> from ata_dev_quirks() when a match on a device model or device model > >>>> and revision is found for a device in the __ata_dev_quirks array. > >>>> > >>>> To implement this function, the ATA_QUIRK_ flags are redefined using > >>>> the new enum ata_quirk which defines the bit shift for each quirk > >>>> flag. The array of strings ata_quirk_names is used to define the name > >>>> of each flag, which are printed by ata_dev_print_quirks(). > >>>> > >>>> Example output for a device listed in the __ata_dev_quirks array and > >>>> which has the ATA_QUIRK_DISABLE flag applied: > >>>> > >>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > >>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable > >>>> [10193.469195] ata1.00: unsupported device, disabling > >>>> [10193.481564] ata1.00: disable device > >>>> > >>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the > >>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a > >>>> build time check that all quirk flags fit within the unsigned int > >>>> (32-bits) quirks field of struct ata_device. > >>>> > >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > >>> > >>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: > >>> libata: Print quirks applied to devices") in libata/for-next. > >>> > >>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is > >>> printed not once, but four times. Is that intentional? > >> > >> Not at all. I tested on x86 with AHCI and see this message only once. So it > >> could be that different drivers may need some tweaks to avoid this spamming. > >> Though it is strange that the initialization or resume path takes this path 4 > >> times, meaning that the quirks are applied 4 times. Need to look into that. > >> What is the driver for rcar-sata ? Compatible string for it would be fine. > > > > drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. > > > > I added a WARN() to ata_dev_quirks() to show backtraces: > > > > Call trace: > > ata_dev_quirks+0x98/0x19c > > ata_dev_configure+0x74/0x12d8 > > ata_eh_recover+0x8d8/0xd08 > > ata_do_eh+0x50/0xa8 > > ata_sff_error_handler+0xd0/0xec > > ata_bmdma_error_handler+0x7c/0x12c > > ata_scsi_port_error_handler+0xc8/0x5f8 > > ata_scsi_error+0x90/0xcc > > scsi_error_handler+0x148/0x308 > > kthread+0xe4/0xf4 > > ret_from_fork+0x10/0x20 > > OK. So it is ata_dev_configure() being called many times from EH. Weird. > But I have not a lot of experience with the bmdma drivers. > Need to look into that. > > In the meantime, can you try this ? > > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, > size_t sz; > char *str; > > - if (!quirks) > + if (!ata_dev_print_info(dev) || !quirks) > return; > > sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; Thanks, that reduces the number of quirk prints from 4 to 2 during boot-up, and from 4 to 0 when resuming from s2idle/s2ram. Gr{oetje,eeting}s, Geert
On 8/1/24 6:07 PM, Geert Uytterhoeven wrote: > Hi Damien, > > On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@kernel.org> wrote: >> On 7/31/24 16:27, Geert Uytterhoeven wrote: >>> On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: >>>> On 7/30/24 19:09, Geert Uytterhoeven wrote: >>>>> On Fri, 26 Jul 2024, Damien Le Moal wrote: >>>>>> Introduce the function ata_dev_print_quirks() to print the quirk flags >>>>>> that will be applied to a scanned device. This new function is called >>>>>> from ata_dev_quirks() when a match on a device model or device model >>>>>> and revision is found for a device in the __ata_dev_quirks array. >>>>>> >>>>>> To implement this function, the ATA_QUIRK_ flags are redefined using >>>>>> the new enum ata_quirk which defines the bit shift for each quirk >>>>>> flag. The array of strings ata_quirk_names is used to define the name >>>>>> of each flag, which are printed by ata_dev_print_quirks(). >>>>>> >>>>>> Example output for a device listed in the __ata_dev_quirks array and >>>>>> which has the ATA_QUIRK_DISABLE flag applied: >>>>>> >>>>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) >>>>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable >>>>>> [10193.469195] ata1.00: unsupported device, disabling >>>>>> [10193.481564] ata1.00: disable device >>>>>> >>>>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the >>>>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a >>>>>> build time check that all quirk flags fit within the unsigned int >>>>>> (32-bits) quirks field of struct ata_device. >>>>>> >>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> >>>>> >>>>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: >>>>> libata: Print quirks applied to devices") in libata/for-next. >>>>> >>>>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is >>>>> printed not once, but four times. Is that intentional? >>>> >>>> Not at all. I tested on x86 with AHCI and see this message only once. So it >>>> could be that different drivers may need some tweaks to avoid this spamming. >>>> Though it is strange that the initialization or resume path takes this path 4 >>>> times, meaning that the quirks are applied 4 times. Need to look into that. >>>> What is the driver for rcar-sata ? Compatible string for it would be fine. >>> >>> drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. >>> >>> I added a WARN() to ata_dev_quirks() to show backtraces: >>> >>> Call trace: >>> ata_dev_quirks+0x98/0x19c >>> ata_dev_configure+0x74/0x12d8 >>> ata_eh_recover+0x8d8/0xd08 >>> ata_do_eh+0x50/0xa8 >>> ata_sff_error_handler+0xd0/0xec >>> ata_bmdma_error_handler+0x7c/0x12c >>> ata_scsi_port_error_handler+0xc8/0x5f8 >>> ata_scsi_error+0x90/0xcc >>> scsi_error_handler+0x148/0x308 >>> kthread+0xe4/0xf4 >>> ret_from_fork+0x10/0x20 >> >> OK. So it is ata_dev_configure() being called many times from EH. Weird. >> But I have not a lot of experience with the bmdma drivers. >> Need to look into that. >> >> In the meantime, can you try this ? >> >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c > >> @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, >> size_t sz; >> char *str; >> >> - if (!quirks) >> + if (!ata_dev_print_info(dev) || !quirks) >> return; >> >> sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; > > Thanks, that reduces the number of quirk prints from 4 to 2 during > boot-up, and from 4 to 0 when resuming from s2idle/s2ram. 2 times on boot... Hmm.. So that means that you are seeing all the probe messages twice (and not just the quirk message), right ? Note that I prepared a better patch for this: commit bc021024de6034a31a818103e4a9845390ba0c47 Author: Damien Le Moal <dlemoal@kernel.org> Date: Thu Aug 1 18:04:22 2024 +0900 ata: libata: Print device quirks only once In ata_dev_print_quirks(), return early if ata_dev_print_info() returns false to avoid printing a device quirks multiple times (that is, each time ata_dev_revalidate() is called). Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Fixes: 58157d607aec ("ata: libata: Print quirks applied to devices") Signed-off-by: Damien Le Moal <dlemoal@kernel.org> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index b4fdb78579c8..3fc9a68d4f45 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -160,7 +160,7 @@ MODULE_DESCRIPTION("Library module for ATA devices"); MODULE_LICENSE("GPL"); MODULE_VERSION(DRV_VERSION); -static inline bool ata_dev_print_info(struct ata_device *dev) +static inline bool ata_dev_print_info(const struct ata_device *dev) { struct ata_eh_context *ehc = &dev->link->eh_context; @@ -4029,7 +4029,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, size_t sz; char *str; - if (!quirks) + if (!ata_dev_print_info(dev) || !quirks) return; sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; But if you prefer to see the quirk message only once, then I will need to change this and use a flag to remember that quirk info has been printed already. But in your case, I suspect you see all probe messages twice, no ?
Hi Damien, On Thu, Aug 1, 2024 at 11:25 AM Damien Le Moal <dlemoal@kernel.org> wrote: > On 8/1/24 6:07 PM, Geert Uytterhoeven wrote: > > On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@kernel.org> wrote: > >> On 7/31/24 16:27, Geert Uytterhoeven wrote: > >>> On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: > >>>> On 7/30/24 19:09, Geert Uytterhoeven wrote: > >>>>> On Fri, 26 Jul 2024, Damien Le Moal wrote: > >>>>>> Introduce the function ata_dev_print_quirks() to print the quirk flags > >>>>>> that will be applied to a scanned device. This new function is called > >>>>>> from ata_dev_quirks() when a match on a device model or device model > >>>>>> and revision is found for a device in the __ata_dev_quirks array. > >>>>>> > >>>>>> To implement this function, the ATA_QUIRK_ flags are redefined using > >>>>>> the new enum ata_quirk which defines the bit shift for each quirk > >>>>>> flag. The array of strings ata_quirk_names is used to define the name > >>>>>> of each flag, which are printed by ata_dev_print_quirks(). > >>>>>> > >>>>>> Example output for a device listed in the __ata_dev_quirks array and > >>>>>> which has the ATA_QUIRK_DISABLE flag applied: > >>>>>> > >>>>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > >>>>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable > >>>>>> [10193.469195] ata1.00: unsupported device, disabling > >>>>>> [10193.481564] ata1.00: disable device > >>>>>> > >>>>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the > >>>>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a > >>>>>> build time check that all quirk flags fit within the unsigned int > >>>>>> (32-bits) quirks field of struct ata_device. > >>>>>> > >>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >>>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > >>>>> > >>>>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: > >>>>> libata: Print quirks applied to devices") in libata/for-next. > >>>>> > >>>>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is > >>>>> printed not once, but four times. Is that intentional? > >>>> > >>>> Not at all. I tested on x86 with AHCI and see this message only once. So it > >>>> could be that different drivers may need some tweaks to avoid this spamming. > >>>> Though it is strange that the initialization or resume path takes this path 4 > >>>> times, meaning that the quirks are applied 4 times. Need to look into that. > >>>> What is the driver for rcar-sata ? Compatible string for it would be fine. > >>> > >>> drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. > >>> > >>> I added a WARN() to ata_dev_quirks() to show backtraces: > >>> > >>> Call trace: > >>> ata_dev_quirks+0x98/0x19c > >>> ata_dev_configure+0x74/0x12d8 > >>> ata_eh_recover+0x8d8/0xd08 > >>> ata_do_eh+0x50/0xa8 > >>> ata_sff_error_handler+0xd0/0xec > >>> ata_bmdma_error_handler+0x7c/0x12c > >>> ata_scsi_port_error_handler+0xc8/0x5f8 > >>> ata_scsi_error+0x90/0xcc > >>> scsi_error_handler+0x148/0x308 > >>> kthread+0xe4/0xf4 > >>> ret_from_fork+0x10/0x20 > >> > >> OK. So it is ata_dev_configure() being called many times from EH. Weird. > >> But I have not a lot of experience with the bmdma drivers. > >> Need to look into that. > >> > >> In the meantime, can you try this ? > >> > >> --- a/drivers/ata/libata-core.c > >> +++ b/drivers/ata/libata-core.c > > > >> @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, > >> size_t sz; > >> char *str; > >> > >> - if (!quirks) > >> + if (!ata_dev_print_info(dev) || !quirks) > >> return; > >> > >> sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; > > > > Thanks, that reduces the number of quirk prints from 4 to 2 during > > boot-up, and from 4 to 0 when resuming from s2idle/s2ram. > > 2 times on boot... Hmm.. So that means that you are seeing all the probe > messages twice (and not just the quirk message), right ? No, I do not see all probe messages twice. $ grep ^ata dmesg: ata1: SATA max UDMA/133 irq 128 lpm-pol 0 ata1: link resume succeeded after 1 retries ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq ata1.00: configured for UDMA/133 Gr{oetje,eeting}s, Geert
On 8/1/24 7:05 PM, Geert Uytterhoeven wrote: > Hi Damien, > > On Thu, Aug 1, 2024 at 11:25 AM Damien Le Moal <dlemoal@kernel.org> wrote: >> On 8/1/24 6:07 PM, Geert Uytterhoeven wrote: >>> On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@kernel.org> wrote: >>>> On 7/31/24 16:27, Geert Uytterhoeven wrote: >>>>> On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: >>>>>> On 7/30/24 19:09, Geert Uytterhoeven wrote: >>>>>>> On Fri, 26 Jul 2024, Damien Le Moal wrote: >>>>>>>> Introduce the function ata_dev_print_quirks() to print the quirk flags >>>>>>>> that will be applied to a scanned device. This new function is called >>>>>>>> from ata_dev_quirks() when a match on a device model or device model >>>>>>>> and revision is found for a device in the __ata_dev_quirks array. >>>>>>>> >>>>>>>> To implement this function, the ATA_QUIRK_ flags are redefined using >>>>>>>> the new enum ata_quirk which defines the bit shift for each quirk >>>>>>>> flag. The array of strings ata_quirk_names is used to define the name >>>>>>>> of each flag, which are printed by ata_dev_print_quirks(). >>>>>>>> >>>>>>>> Example output for a device listed in the __ata_dev_quirks array and >>>>>>>> which has the ATA_QUIRK_DISABLE flag applied: >>>>>>>> >>>>>>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) >>>>>>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable >>>>>>>> [10193.469195] ata1.00: unsupported device, disabling >>>>>>>> [10193.481564] ata1.00: disable device >>>>>>>> >>>>>>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the >>>>>>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a >>>>>>>> build time check that all quirk flags fit within the unsigned int >>>>>>>> (32-bits) quirks field of struct ata_device. >>>>>>>> >>>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> >>>>>>> >>>>>>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: >>>>>>> libata: Print quirks applied to devices") in libata/for-next. >>>>>>> >>>>>>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is >>>>>>> printed not once, but four times. Is that intentional? >>>>>> >>>>>> Not at all. I tested on x86 with AHCI and see this message only once. So it >>>>>> could be that different drivers may need some tweaks to avoid this spamming. >>>>>> Though it is strange that the initialization or resume path takes this path 4 >>>>>> times, meaning that the quirks are applied 4 times. Need to look into that. >>>>>> What is the driver for rcar-sata ? Compatible string for it would be fine. >>>>> >>>>> drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. >>>>> >>>>> I added a WARN() to ata_dev_quirks() to show backtraces: >>>>> >>>>> Call trace: >>>>> ata_dev_quirks+0x98/0x19c >>>>> ata_dev_configure+0x74/0x12d8 >>>>> ata_eh_recover+0x8d8/0xd08 >>>>> ata_do_eh+0x50/0xa8 >>>>> ata_sff_error_handler+0xd0/0xec >>>>> ata_bmdma_error_handler+0x7c/0x12c >>>>> ata_scsi_port_error_handler+0xc8/0x5f8 >>>>> ata_scsi_error+0x90/0xcc >>>>> scsi_error_handler+0x148/0x308 >>>>> kthread+0xe4/0xf4 >>>>> ret_from_fork+0x10/0x20 >>>> >>>> OK. So it is ata_dev_configure() being called many times from EH. Weird. >>>> But I have not a lot of experience with the bmdma drivers. >>>> Need to look into that. >>>> >>>> In the meantime, can you try this ? >>>> >>>> --- a/drivers/ata/libata-core.c >>>> +++ b/drivers/ata/libata-core.c >>> >>>> @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, >>>> size_t sz; >>>> char *str; >>>> >>>> - if (!quirks) >>>> + if (!ata_dev_print_info(dev) || !quirks) >>>> return; >>>> >>>> sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; >>> >>> Thanks, that reduces the number of quirk prints from 4 to 2 during >>> boot-up, and from 4 to 0 when resuming from s2idle/s2ram. >> >> 2 times on boot... Hmm.. So that means that you are seeing all the probe >> messages twice (and not just the quirk message), right ? > > No, I do not see all probe messages twice. > > $ grep ^ata dmesg: > > ata1: SATA max UDMA/133 irq 128 lpm-pol 0 > ata1: link resume succeeded after 1 retries > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 > ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) > ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: configured for UDMA/133 Odd. I am missing something... Let me dig into that.
On 8/1/24 7:05 PM, Geert Uytterhoeven wrote: > Hi Damien, > > On Thu, Aug 1, 2024 at 11:25 AM Damien Le Moal <dlemoal@kernel.org> wrote: >> On 8/1/24 6:07 PM, Geert Uytterhoeven wrote: >>> On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@kernel.org> wrote: >>>> On 7/31/24 16:27, Geert Uytterhoeven wrote: >>>>> On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: >>>>>> On 7/30/24 19:09, Geert Uytterhoeven wrote: >>>>>>> On Fri, 26 Jul 2024, Damien Le Moal wrote: >>>>>>>> Introduce the function ata_dev_print_quirks() to print the quirk flags >>>>>>>> that will be applied to a scanned device. This new function is called >>>>>>>> from ata_dev_quirks() when a match on a device model or device model >>>>>>>> and revision is found for a device in the __ata_dev_quirks array. >>>>>>>> >>>>>>>> To implement this function, the ATA_QUIRK_ flags are redefined using >>>>>>>> the new enum ata_quirk which defines the bit shift for each quirk >>>>>>>> flag. The array of strings ata_quirk_names is used to define the name >>>>>>>> of each flag, which are printed by ata_dev_print_quirks(). >>>>>>>> >>>>>>>> Example output for a device listed in the __ata_dev_quirks array and >>>>>>>> which has the ATA_QUIRK_DISABLE flag applied: >>>>>>>> >>>>>>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) >>>>>>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable >>>>>>>> [10193.469195] ata1.00: unsupported device, disabling >>>>>>>> [10193.481564] ata1.00: disable device >>>>>>>> >>>>>>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the >>>>>>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a >>>>>>>> build time check that all quirk flags fit within the unsigned int >>>>>>>> (32-bits) quirks field of struct ata_device. >>>>>>>> >>>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>>>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> >>>>>>> >>>>>>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: >>>>>>> libata: Print quirks applied to devices") in libata/for-next. >>>>>>> >>>>>>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is >>>>>>> printed not once, but four times. Is that intentional? >>>>>> >>>>>> Not at all. I tested on x86 with AHCI and see this message only once. So it >>>>>> could be that different drivers may need some tweaks to avoid this spamming. >>>>>> Though it is strange that the initialization or resume path takes this path 4 >>>>>> times, meaning that the quirks are applied 4 times. Need to look into that. >>>>>> What is the driver for rcar-sata ? Compatible string for it would be fine. >>>>> >>>>> drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. >>>>> >>>>> I added a WARN() to ata_dev_quirks() to show backtraces: >>>>> >>>>> Call trace: >>>>> ata_dev_quirks+0x98/0x19c >>>>> ata_dev_configure+0x74/0x12d8 >>>>> ata_eh_recover+0x8d8/0xd08 >>>>> ata_do_eh+0x50/0xa8 >>>>> ata_sff_error_handler+0xd0/0xec >>>>> ata_bmdma_error_handler+0x7c/0x12c >>>>> ata_scsi_port_error_handler+0xc8/0x5f8 >>>>> ata_scsi_error+0x90/0xcc >>>>> scsi_error_handler+0x148/0x308 >>>>> kthread+0xe4/0xf4 >>>>> ret_from_fork+0x10/0x20 >>>> >>>> OK. So it is ata_dev_configure() being called many times from EH. Weird. >>>> But I have not a lot of experience with the bmdma drivers. >>>> Need to look into that. >>>> >>>> In the meantime, can you try this ? >>>> >>>> --- a/drivers/ata/libata-core.c >>>> +++ b/drivers/ata/libata-core.c >>> >>>> @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, >>>> size_t sz; >>>> char *str; >>>> >>>> - if (!quirks) >>>> + if (!ata_dev_print_info(dev) || !quirks) >>>> return; >>>> >>>> sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; >>> >>> Thanks, that reduces the number of quirk prints from 4 to 2 during >>> boot-up, and from 4 to 0 when resuming from s2idle/s2ram. >> >> 2 times on boot... Hmm.. So that means that you are seeing all the probe >> messages twice (and not just the quirk message), right ? > > No, I do not see all probe messages twice. > > $ grep ^ata dmesg: > > ata1: SATA max UDMA/133 irq 128 lpm-pol 0 > ata1: link resume succeeded after 1 retries > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 > ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) > ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > ata1.00: configured for UDMA/133 OK. This path should get rid of the useless extra print: commit 3c65fcbf942c26ece6d1efef7ad1405c0163575f Author: Damien Le Moal <dlemoal@kernel.org> Date: Thu Aug 1 18:04:22 2024 +0900 ata: libata: Print device quirks only once In ata_dev_print_quirks(), return early if ata_dev_print_info() returns false or if we already printed quirk information. This is to avoid printing a device quirks multiple times (that is, each time ata_dev_revalidate() is called). To remember if ata_dev_print_quirks() was already executed, define the EH context flag ATA_EHI_DID_QUIRK_PRINT and set this flag in ata_dev_print_quirks(). Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Fixes: 58157d607aec ("ata: libata: Print quirks applied to devices") Signed-off-by: Damien Le Moal <dlemoal@kernel.org> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index b4fdb78579c8..2309927b7c23 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -160,7 +160,7 @@ MODULE_DESCRIPTION("Library module for ATA devices"); MODULE_LICENSE("GPL"); MODULE_VERSION(DRV_VERSION); -static inline bool ata_dev_print_info(struct ata_device *dev) +static inline bool ata_dev_print_info(const struct ata_device *dev) { struct ata_eh_context *ehc = &dev->link->eh_context; @@ -4025,10 +4025,16 @@ static void ata_dev_print_quirks(const struct ata_device *dev, const char *model, const char *rev, unsigned int quirks) { + struct ata_eh_context *ehc = &dev->link->eh_context; int n = 0, i; size_t sz; char *str; + if (!ata_dev_print_info(dev) || ehc->i.flags & ATA_EHI_DID_QUIRK_PRINT) + return; + + ehc->i.flags |= ATA_EHI_DID_QUIRK_PRINT; + if (!quirks) return; diff --git a/include/linux/libata.h b/include/linux/libata.h index d5446e18d9df..f8fb9bc7c743 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -378,6 +378,7 @@ enum { ATA_EHI_PRINTINFO = (1 << 18), /* print configuration info */ ATA_EHI_SETMODE = (1 << 19), /* configure transfer mode */ ATA_EHI_POST_SETMODE = (1 << 20), /* revalidating after setmode */ + ATA_EHI_DID_QUIRK_PRINT = (1 << 21), /* already printed quirk info */ ATA_EHI_DID_RESET = ATA_EHI_DID_SOFTRESET | ATA_EHI_DID_HARDRESET,
Hi Damien, On Thu, Aug 1, 2024 at 12:42 PM Damien Le Moal <dlemoal@kernel.org> wrote: > On 8/1/24 7:05 PM, Geert Uytterhoeven wrote: > > On Thu, Aug 1, 2024 at 11:25 AM Damien Le Moal <dlemoal@kernel.org> wrote: > >> On 8/1/24 6:07 PM, Geert Uytterhoeven wrote: > >>> On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@kernel.org> wrote: > >>>> On 7/31/24 16:27, Geert Uytterhoeven wrote: > >>>>> On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@kernel.org> wrote: > >>>>>> On 7/30/24 19:09, Geert Uytterhoeven wrote: > >>>>>>> On Fri, 26 Jul 2024, Damien Le Moal wrote: > >>>>>>>> Introduce the function ata_dev_print_quirks() to print the quirk flags > >>>>>>>> that will be applied to a scanned device. This new function is called > >>>>>>>> from ata_dev_quirks() when a match on a device model or device model > >>>>>>>> and revision is found for a device in the __ata_dev_quirks array. > >>>>>>>> > >>>>>>>> To implement this function, the ATA_QUIRK_ flags are redefined using > >>>>>>>> the new enum ata_quirk which defines the bit shift for each quirk > >>>>>>>> flag. The array of strings ata_quirk_names is used to define the name > >>>>>>>> of each flag, which are printed by ata_dev_print_quirks(). > >>>>>>>> > >>>>>>>> Example output for a device listed in the __ata_dev_quirks array and > >>>>>>>> which has the ATA_QUIRK_DISABLE flag applied: > >>>>>>>> > >>>>>>>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > >>>>>>>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable > >>>>>>>> [10193.469195] ata1.00: unsupported device, disabling > >>>>>>>> [10193.481564] ata1.00: disable device > >>>>>>>> > >>>>>>>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the > >>>>>>>> last quirk flag defined. This value is used in ata_dev_quirks() to add a > >>>>>>>> build time check that all quirk flags fit within the unsigned int > >>>>>>>> (32-bits) quirks field of struct ata_device. > >>>>>>>> > >>>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >>>>>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > >>>>>>> > >>>>>>> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata: > >>>>>>> libata: Print quirks applied to devices") in libata/for-next. > >>>>>>> > >>>>>>> During boot-up on Salvator-XS (using rcar-sata), the quirk info is > >>>>>>> printed not once, but four times. Is that intentional? > >>>>>> > >>>>>> Not at all. I tested on x86 with AHCI and see this message only once. So it > >>>>>> could be that different drivers may need some tweaks to avoid this spamming. > >>>>>> Though it is strange that the initialization or resume path takes this path 4 > >>>>>> times, meaning that the quirks are applied 4 times. Need to look into that. > >>>>>> What is the driver for rcar-sata ? Compatible string for it would be fine. > >>>>> > >>>>> drivers/ata/sata_rcar.c, using renesas,rcar-gen3-sata. > >>>>> > >>>>> I added a WARN() to ata_dev_quirks() to show backtraces: > >>>>> > >>>>> Call trace: > >>>>> ata_dev_quirks+0x98/0x19c > >>>>> ata_dev_configure+0x74/0x12d8 > >>>>> ata_eh_recover+0x8d8/0xd08 > >>>>> ata_do_eh+0x50/0xa8 > >>>>> ata_sff_error_handler+0xd0/0xec > >>>>> ata_bmdma_error_handler+0x7c/0x12c > >>>>> ata_scsi_port_error_handler+0xc8/0x5f8 > >>>>> ata_scsi_error+0x90/0xcc > >>>>> scsi_error_handler+0x148/0x308 > >>>>> kthread+0xe4/0xf4 > >>>>> ret_from_fork+0x10/0x20 > >>>> > >>>> OK. So it is ata_dev_configure() being called many times from EH. Weird. > >>>> But I have not a lot of experience with the bmdma drivers. > >>>> Need to look into that. > >>>> > >>>> In the meantime, can you try this ? > >>>> > >>>> --- a/drivers/ata/libata-core.c > >>>> +++ b/drivers/ata/libata-core.c > >>> > >>>> @@ -4087,7 +4087,7 @@ static void ata_dev_print_quirks(const struct ata_device *dev, > >>>> size_t sz; > >>>> char *str; > >>>> > >>>> - if (!quirks) > >>>> + if (!ata_dev_print_info(dev) || !quirks) > >>>> return; > >>>> > >>>> sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; > >>> > >>> Thanks, that reduces the number of quirk prints from 4 to 2 during > >>> boot-up, and from 4 to 0 when resuming from s2idle/s2ram. > >> > >> 2 times on boot... Hmm.. So that means that you are seeing all the probe > >> messages twice (and not just the quirk message), right ? > > > > No, I do not see all probe messages twice. > > > > $ grep ^ata dmesg: > > > > ata1: SATA max UDMA/133 irq 128 lpm-pol 0 > > ata1: link resume succeeded after 1 retries > > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > > ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > > ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 > > ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) > > ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq > > ata1.00: configured for UDMA/133 > > OK. This path should get rid of the useless extra print: Unsurprisingly, it does ;-) > > commit 3c65fcbf942c26ece6d1efef7ad1405c0163575f > Author: Damien Le Moal <dlemoal@kernel.org> > Date: Thu Aug 1 18:04:22 2024 +0900 > > ata: libata: Print device quirks only once > > In ata_dev_print_quirks(), return early if ata_dev_print_info() returns > false or if we already printed quirk information. This is to avoid > printing a device quirks multiple times (that is, each time > ata_dev_revalidate() is called). > > To remember if ata_dev_print_quirks() was already executed, define the > EH context flag ATA_EHI_DID_QUIRK_PRINT and set this flag in > ata_dev_print_quirks(). > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Fixes: 58157d607aec ("ata: libata: Print quirks applied to devices") > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 19b041bd7588..fc9fcfda42b8 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -3988,6 +3988,69 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class, return rc; } +static const char * const ata_quirk_names[] = { + [__ATA_QUIRK_DIAGNOSTIC] = "diagnostic", + [__ATA_QUIRK_NODMA] = "nodma", + [__ATA_QUIRK_NONCQ] = "noncq", + [__ATA_QUIRK_MAX_SEC_128] = "maxsec128", + [__ATA_QUIRK_BROKEN_HPA] = "brokenhpa", + [__ATA_QUIRK_DISABLE] = "disable", + [__ATA_QUIRK_HPA_SIZE] = "hpasize", + [__ATA_QUIRK_IVB] = "ivb", + [__ATA_QUIRK_STUCK_ERR] = "stuckerr", + [__ATA_QUIRK_BRIDGE_OK] = "bridgeok", + [__ATA_QUIRK_ATAPI_MOD16_DMA] = "atapimod16dma", + [__ATA_QUIRK_FIRMWARE_WARN] = "firmwarewarn", + [__ATA_QUIRK_1_5_GBPS] = "1.5gbps", + [__ATA_QUIRK_NOSETXFER] = "nosetxfer", + [__ATA_QUIRK_BROKEN_FPDMA_AA] = "brokenfpdmaaa", + [__ATA_QUIRK_DUMP_ID] = "dumpid", + [__ATA_QUIRK_MAX_SEC_LBA48] = "maxseclba48", + [__ATA_QUIRK_ATAPI_DMADIR] = "atapidmadir", + [__ATA_QUIRK_NO_NCQ_TRIM] = "noncqtrim", + [__ATA_QUIRK_NOLPM] = "nolpm", + [__ATA_QUIRK_WD_BROKEN_LPM] = "wdbrokenlpm", + [__ATA_QUIRK_ZERO_AFTER_TRIM] = "zeroaftertrim", + [__ATA_QUIRK_NO_DMA_LOG] = "nodmalog", + [__ATA_QUIRK_NOTRIM] = "notrim", + [__ATA_QUIRK_MAX_SEC_1024] = "maxsec1024", + [__ATA_QUIRK_MAX_TRIM_128M] = "maxtrim128m", + [__ATA_QUIRK_NO_NCQ_ON_ATI] = "noncqonati", + [__ATA_QUIRK_NO_ID_DEV_LOG] = "noiddevlog", + [__ATA_QUIRK_NO_LOG_DIR] = "nologdir", + [__ATA_QUIRK_NO_FUA] = "nofua", +}; + +static void ata_dev_print_quirks(const struct ata_device *dev, + const char *model, const char *rev, + unsigned int quirks) +{ + int n = 0, i; + size_t sz; + char *str; + + if (!quirks) + return; + + sz = 64 + ARRAY_SIZE(ata_quirk_names) * 16; + str = kmalloc(sz, GFP_KERNEL); + if (!str) + return; + + n = snprintf(str, sz, "Model '%s', rev '%s', applying quirks:", + model, rev); + + for (i = 0; i < ARRAY_SIZE(ata_quirk_names); i++) { + if (quirks & (1U << i)) + n += snprintf(str + n, sz - n, + " %s", ata_quirk_names[i]); + } + + ata_dev_warn(dev, "%s\n", str); + + kfree(str); +} + struct ata_dev_quirks_entry { const char *model_num; const char *model_rev; @@ -4273,15 +4336,18 @@ static unsigned int ata_dev_quirks(const struct ata_device *dev) unsigned char model_rev[ATA_ID_FW_REV_LEN + 1]; const struct ata_dev_quirks_entry *ad = __ata_dev_quirks; + /* dev->quirks is an unsigned int. */ + BUILD_BUG_ON(__ATA_QUIRK_MAX > 32); + ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num)); ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev)); while (ad->model_num) { - if (glob_match(ad->model_num, model_num)) { - if (ad->model_rev == NULL) - return ad->quirks; - if (glob_match(ad->model_rev, model_rev)) - return ad->quirks; + if (glob_match(ad->model_num, model_num) && + (!ad->model_rev || glob_match(ad->model_rev, model_rev))) { + ata_dev_print_quirks(dev, model_num, model_rev, + ad->quirks); + return ad->quirks; } ad++; } diff --git a/include/linux/libata.h b/include/linux/libata.h index 05dd7038ab30..d598ef690e50 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -55,6 +55,46 @@ /* defines only for the constants which don't work well as enums */ #define ATA_TAG_POISON 0xfafbfcfdU +/* + * Quirk flags bits. + * ata_device->quirks is an unsigned int, so __ATA_QUIRK_MAX must not exceed 32. + */ +enum ata_quirks { + __ATA_QUIRK_DIAGNOSTIC, /* Failed boot diag */ + __ATA_QUIRK_NODMA, /* DMA problems */ + __ATA_QUIRK_NONCQ, /* Don't use NCQ */ + __ATA_QUIRK_MAX_SEC_128, /* Limit max sects to 128 */ + __ATA_QUIRK_BROKEN_HPA, /* Broken HPA */ + __ATA_QUIRK_DISABLE, /* Disable it */ + __ATA_QUIRK_HPA_SIZE, /* Native size off by one */ + __ATA_QUIRK_IVB, /* cbl det validity bit bugs */ + __ATA_QUIRK_STUCK_ERR, /* Stuck ERR on next PACKET */ + __ATA_QUIRK_BRIDGE_OK, /* No bridge limits */ + __ATA_QUIRK_ATAPI_MOD16_DMA, /* Use ATAPI DMA for commands that */ + /* are not a multiple of 16 bytes */ + __ATA_QUIRK_FIRMWARE_WARN, /* Firmware update warning */ + __ATA_QUIRK_1_5_GBPS, /* Force 1.5 Gbps */ + __ATA_QUIRK_NOSETXFER, /* Skip SETXFER, SATA only */ + __ATA_QUIRK_BROKEN_FPDMA_AA, /* Skip AA */ + __ATA_QUIRK_DUMP_ID, /* Dump IDENTIFY data */ + __ATA_QUIRK_MAX_SEC_LBA48, /* Set max sects to 65535 */ + __ATA_QUIRK_ATAPI_DMADIR, /* Device requires dmadir */ + __ATA_QUIRK_NO_NCQ_TRIM, /* Do not use queued TRIM */ + __ATA_QUIRK_NOLPM, /* Do not use LPM */ + __ATA_QUIRK_WD_BROKEN_LPM, /* Some WDs have broken LPM */ + __ATA_QUIRK_ZERO_AFTER_TRIM, /* Guarantees zero after trim */ + __ATA_QUIRK_NO_DMA_LOG, /* Do not use DMA for log read */ + __ATA_QUIRK_NOTRIM, /* Do not use TRIM */ + __ATA_QUIRK_MAX_SEC_1024, /* Limit max sects to 1024 */ + __ATA_QUIRK_MAX_TRIM_128M, /* Limit max trim size to 128M */ + __ATA_QUIRK_NO_NCQ_ON_ATI, /* Disable NCQ on ATI chipset */ + __ATA_QUIRK_NO_ID_DEV_LOG, /* Identify device log missing */ + __ATA_QUIRK_NO_LOG_DIR, /* Do not read log directory */ + __ATA_QUIRK_NO_FUA, /* Do not use FUA */ + + __ATA_QUIRK_MAX, +}; + enum { /* various global constants */ LIBATA_MAX_PRD = ATA_MAX_PRD / 2, @@ -366,40 +406,38 @@ enum { * Quirk flags: may be set by libata or controller drivers on drives. * Some quirks may be drive/controller pair dependent. */ - ATA_QUIRK_DIAGNOSTIC = (1 << 0), /* Failed boot diag */ - ATA_QUIRK_NODMA = (1 << 1), /* DMA problems */ - ATA_QUIRK_NONCQ = (1 << 2), /* Do not use NCQ */ - ATA_QUIRK_MAX_SEC_128 = (1 << 3), /* Limit max sects to 128 */ - ATA_QUIRK_BROKEN_HPA = (1 << 4), /* Broken HPA */ - ATA_QUIRK_DISABLE = (1 << 5), /* Disable it */ - ATA_QUIRK_HPA_SIZE = (1 << 6), /* Native size off by one */ - ATA_QUIRK_IVB = (1 << 8), /* CBL det validity bit bugs */ - ATA_QUIRK_STUCK_ERR = (1 << 9), /* Stuck ERR on next PACKET */ - ATA_QUIRK_BRIDGE_OK = (1 << 10), /* No bridge limits */ - ATA_QUIRK_ATAPI_MOD16_DMA = (1 << 11), /* Use ATAPI DMA for commands */ - /* not multiple of 16 bytes */ - ATA_QUIRK_FIRMWARE_WARN = (1 << 12), /* Firmware update warning */ - ATA_QUIRK_1_5_GBPS = (1 << 13), /* Force 1.5 Gbps */ - ATA_QUIRK_NOSETXFER = (1 << 14), /* Skip SETXFER, SATA only */ - ATA_QUIRK_BROKEN_FPDMA_AA = (1 << 15), /* Skip AA */ - ATA_QUIRK_DUMP_ID = (1 << 16), /* Dump IDENTIFY data */ - ATA_QUIRK_MAX_SEC_LBA48 = (1 << 17), /* Set max sects to 65535 */ - ATA_QUIRK_ATAPI_DMADIR = (1 << 18), /* Device requires dmadir */ - ATA_QUIRK_NO_NCQ_TRIM = (1 << 19), /* Do not use queued TRIM */ - ATA_QUIRK_NOLPM = (1 << 20), /* Do not use LPM */ - ATA_QUIRK_WD_BROKEN_LPM = (1 << 21), /* Some WDs have broken LPM */ - ATA_QUIRK_ZERO_AFTER_TRIM = (1 << 22), /* Guarantees zero after trim */ - ATA_QUIRK_NO_DMA_LOG = (1 << 23), /* Do not use DMA for log read */ - ATA_QUIRK_NOTRIM = (1 << 24), /* Do not use TRIM */ - ATA_QUIRK_MAX_SEC_1024 = (1 << 25), /* Limit max sects to 1024 */ - ATA_QUIRK_MAX_TRIM_128M = (1 << 26), /* Limit max trim size to 128M */ - ATA_QUIRK_NO_NCQ_ON_ATI = (1 << 27), /* Disable NCQ on ATI chipset */ - ATA_QUIRK_NO_ID_DEV_LOG = (1 << 28), /* Identify device log missing */ - ATA_QUIRK_NO_LOG_DIR = (1 << 29), /* Do not read log directory */ - ATA_QUIRK_NO_FUA = (1 << 30), /* Do not use FUA */ - - /* DMA mask for user DMA control: User visible values; DO NOT - renumber */ + ATA_QUIRK_DIAGNOSTIC = (1U << __ATA_QUIRK_DIAGNOSTIC), + ATA_QUIRK_NODMA = (1U << __ATA_QUIRK_NODMA), + ATA_QUIRK_NONCQ = (1U << __ATA_QUIRK_NONCQ), + ATA_QUIRK_MAX_SEC_128 = (1U << __ATA_QUIRK_MAX_SEC_128), + ATA_QUIRK_BROKEN_HPA = (1U << __ATA_QUIRK_BROKEN_HPA), + ATA_QUIRK_DISABLE = (1U << __ATA_QUIRK_DISABLE), + ATA_QUIRK_HPA_SIZE = (1U << __ATA_QUIRK_HPA_SIZE), + ATA_QUIRK_IVB = (1U << __ATA_QUIRK_IVB), + ATA_QUIRK_STUCK_ERR = (1U << __ATA_QUIRK_STUCK_ERR), + ATA_QUIRK_BRIDGE_OK = (1U << __ATA_QUIRK_BRIDGE_OK), + ATA_QUIRK_ATAPI_MOD16_DMA = (1U << __ATA_QUIRK_ATAPI_MOD16_DMA), + ATA_QUIRK_FIRMWARE_WARN = (1U << __ATA_QUIRK_FIRMWARE_WARN), + ATA_QUIRK_1_5_GBPS = (1U << __ATA_QUIRK_1_5_GBPS), + ATA_QUIRK_NOSETXFER = (1U << __ATA_QUIRK_NOSETXFER), + ATA_QUIRK_BROKEN_FPDMA_AA = (1U << __ATA_QUIRK_BROKEN_FPDMA_AA), + ATA_QUIRK_DUMP_ID = (1U << __ATA_QUIRK_DUMP_ID), + ATA_QUIRK_MAX_SEC_LBA48 = (1U << __ATA_QUIRK_MAX_SEC_LBA48), + ATA_QUIRK_ATAPI_DMADIR = (1U << __ATA_QUIRK_ATAPI_DMADIR), + ATA_QUIRK_NO_NCQ_TRIM = (1U << __ATA_QUIRK_NO_NCQ_TRIM), + ATA_QUIRK_NOLPM = (1U << __ATA_QUIRK_NOLPM), + ATA_QUIRK_WD_BROKEN_LPM = (1U << __ATA_QUIRK_WD_BROKEN_LPM), + ATA_QUIRK_ZERO_AFTER_TRIM = (1U << __ATA_QUIRK_ZERO_AFTER_TRIM), + ATA_QUIRK_NO_DMA_LOG = (1U << __ATA_QUIRK_NO_DMA_LOG), + ATA_QUIRK_NOTRIM = (1U << __ATA_QUIRK_NOTRIM), + ATA_QUIRK_MAX_SEC_1024 = (1U << __ATA_QUIRK_MAX_SEC_1024), + ATA_QUIRK_MAX_TRIM_128M = (1U << __ATA_QUIRK_MAX_TRIM_128M), + ATA_QUIRK_NO_NCQ_ON_ATI = (1U << __ATA_QUIRK_NO_NCQ_ON_ATI), + ATA_QUIRK_NO_ID_DEV_LOG = (1U << __ATA_QUIRK_NO_ID_DEV_LOG), + ATA_QUIRK_NO_LOG_DIR = (1U << __ATA_QUIRK_NO_LOG_DIR), + ATA_QUIRK_NO_FUA = (1U << __ATA_QUIRK_NO_FUA), + + /* User visible DMA mask for DMA control. DO NOT renumber. */ ATA_DMA_MASK_ATA = (1 << 0), /* DMA on ATA Disk */ ATA_DMA_MASK_ATAPI = (1 << 1), /* DMA on ATAPI */ ATA_DMA_MASK_CFA = (1 << 2), /* DMA on CF Card */