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@xxxxxxxxxx> wrote:
>> On 8/1/24 6:07 PM, Geert Uytterhoeven wrote:
>>> On Wed, Jul 31, 2024 at 11:08 AM Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>>>> On 7/31/24 16:27, Geert Uytterhoeven wrote:
>>>>> On Wed, Jul 31, 2024 at 1:39 AM Damien Le Moal <dlemoal@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>>>> Reviewed-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx>
>>>>>>>
>>>>>>> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxx>
Fixes: 58157d607aec ("ata: libata: Print quirks applied to devices")
Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
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,
--
Damien Le Moal
Western Digital Research
