On 7/31/24 16:27, Geert Uytterhoeven wrote:
> Hi Damien,
>
> 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 ?
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.
--
Damien Le Moal
Western Digital Research
