On 03 February, 2022 - Damien Le Moal wrote:
> On 2/3/22 18:11, Anton Lundin wrote:
> > On 03 February, 2022 - Damien Le Moal wrote:
> >
> >> On 2/3/22 17:32, Anton Lundin wrote:
> >>> On 03 February, 2022 - Damien Le Moal wrote:
> >>>
> >>>> On 2022/02/02 22:54, Anton Lundin wrote:
> >>>>> On 02 February, 2022 - Anton Lundin wrote:
> >>>>>
> >>>>>> On 02 February, 2022 - Damien Le Moal wrote:
> >>>>>>
> >>>>>>> On 2022/02/02 21:25, Anton Lundin wrote:
> >>>>>>>> On 02 February, 2022 - Damien Le Moal wrote:
> >>>>>>>>
> >>>>>>>>> On 2/2/22 19:05, Anton Lundin wrote:
> >>>>>>>>>> Back in 06f6c4c6c3e8 ("ata: libata: add missing ata_identify_page_supported() calls")
> >>>>>>>>>> a read of ATA_LOG_DIRECTORY page was added. This caused the
> >>>>>>>>>> SATADOM-ML 3ME to lock up.
> >>>>>>>>>>
> >>>>>>>>>> In 636f6e2af4fb ("libata: add horkage for missing Identify Device log")
> >>>>>>>>>> a flag was added to cache if a device supports this or not.
> >>>>>>>>>>
> >>>>>>>>>> This adds a blacklist entry which flags that these devices doesn't
> >>>>>>>>>> support that call and shouldn't be issued that call.
> >>>>>>>>>>
> >>>>>>>>>> Cc: stable@xxxxxxxxxxxxxxx # v5.10+
> >>>>>>>>>> Signed-off-by: Anton Lundin <glance@xxxxxxxxxx>
> >>>>>>>>>> Depends-on: 636f6e2af4fb ("libata: add horkage for missing Identify Device log")
> >>>>>>>>>
> >>>>>>>>> I do not think so. See below.
> >>>>>>>>>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/ata/libata-core.c | 7 +++++++
> >>>>>>>>>> 1 file changed, 7 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> >>>>>>>>>> index 87d36b29ca5f..e024af9f33d0 100644
> >>>>>>>>>> --- a/drivers/ata/libata-core.c
> >>>>>>>>>> +++ b/drivers/ata/libata-core.c
> >>>>>>>>>> @@ -4070,6 +4070,13 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
> >>>>>>>>>> { "WDC WD3000JD-*", NULL, ATA_HORKAGE_WD_BROKEN_LPM },
> >>>>>>>>>> { "WDC WD3200JD-*", NULL, ATA_HORKAGE_WD_BROKEN_LPM },
> >>>>>>>>>>
> >>>>>>>>>> + /*
> >>>>>>>>>> + * This sata dom goes on a walkabout when it sees the
> >>>>>>>>>> + * ATA_LOG_DIRECTORY read request so ensure we don't issue such a
> >>>>>>>>>> + * request to these devices.
> >>>>>>>>>> + */
> >>>>>>>>>> + { "SATADOM-ML 3ME", NULL, ATA_HORKAGE_NO_ID_DEV_LOG },
> >>>>>>>>>
> >>>>>>>>> This flag only disables trying to access the identify device log page,
> >>>>>>>>> it does *not* avoid access to the log directory log page in general. The
> >>>>>>>>> log directory will still be consulted for other log pages beside the
> >>>>>>>>> identify device log page, from any function that calls
> >>>>>>>>> ata_log_supported() (e.g. ata_dev_config_ncq_send_recv() and
> >>>>>>>>> ata_dev_config_ncq_non_data())
> >>>>>>>>
> >>>>>>>> Non of those code paths are called for this device, probably due to some
> >>>>>>>> other flag disqualifying them.
> >>>>>>>>
> >>>>>>>>> So it will be a lot more solid to define a ATA_HORKAGE_NO_LOG_DIR flag
> >>>>>>>>> and test for it in ata_log_supported(), completely preventing any access
> >>>>>>>>> to the log directory page for this drive type.
> >>>>>>>>
> >>>>>>>> That was my first thought but then I found ATA_HORKAGE_NO_ID_DEV_LOG
> >>>>>>>> which was in the calling path that actually triggered this issue.
> >>>>>>>>
> >>>>>>>> But, yes, I totally agree that's a more solid solution preventing this
> >>>>>>>> kind of issue to crop up again.
> >>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> /* End Marker */
> >>>>>>>>>> { }
> >>>>>>>>>> };
> >>>>>>>>>
> >>>>>>>>> Note: if you need this fix sent to linux-stable, add "Cc: stable@..."
> >>>>>>>>> and a Fixes tag.
> >>>>>>>>
> >>>>>>>> I'd think it's fitting to send it to linux-stable, because it prevents
> >>>>>>>> those DOM's from working in v5.15.5+.
> >>>>>>>>
> >>>>>>>> Ok. I must have missed that part when I read submitting-patches doc.
> >>>>>>>>
> >>>>>>>> I'll rework and re-submit the patch.
> >>>>>>>
> >>>>>>> I sent you a draft patch. Please try it.
> >>>>>>
> >>>>>> Works like a charm.
> >>>>>>
> >>>>>>> Also, to confirm if the log directory log page is indeed the page that locks up
> >>>>>>> the drive, can you try this command:
> >>>>>>>
> >>>>>>> sg_sat_read_gplog --dma --log=0 --page=0 --readonly
> >>>>>>
> >>>>>> # sg_sat_read_gplog --dma --log=0 --page=0 --readonly /dev/sda
> >>>>>> ATA PASS-THROUGH (16), bad field in cdb
> >>>>>> sg_sat_read_gplog failed: Illegal request
> >>>>>>
> >>>>>>> and
> >>>>>>>
> >>>>>>> sg_sat_read_gplog --log=0 --page=0 --readonly
> >>>>>>
> >>>>>> # sg_sat_read_gplog --log=0 --page=0 --readonly /dev/sda
> >>>>>> 00 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 08 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 10 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 18 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 20 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 28 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 30 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 38 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 40 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 48 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 50 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 58 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 60 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 68 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 70 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 78 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 80 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 88 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 90 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> 98 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> a0 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> a8 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> b0 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> b8 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> c0 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> c8 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> d0 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> d8 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> e0 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> e8 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> f0 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>> f8 0000 0000 0000 0000 0000 0000 0000 0000 .. .. .. .. .. .. .. ..
> >>>>>>
> >>>>>> Mind you, this with a patched kernel, if that affects anything.
> >>>>>
> >>>>> Without a patched kernel, (grabbed os with a 4.19.94 kernel) the command
> >>>>> hangs for a while:
> >>>>>
> >>>>> # time sg_sat_read_gplog --log=0 --page=0 --readonly /dev/sda
> >>>>> ATA PASS-THROUGH (16), bad field in cdb
> >>>>> sg_sat_read_gplog failed: Illegal request
> >>>>>
> >>>>> real 0m28.337s
> >>>>> user 0m0.000s
> >>>>> sys 0m0.001s
> >>>>>
> >>>>> and logs the following in the kernel log:
> >>>>>
> >>>>> ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> >>>>> ata1.00: failed to IDENTIFY (INIT_DEV_PARAMS failed, err_mask=0x80)
> >>>>> ata1.00: revalidation failed (errno=-5)
> >>>>> ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> >>>>> ata1.00: configured for UDMA/133
> >>>>>
> >>>>>
> >>>>> But after that 30s walkabout and whatever the kernel does the device
> >>>>> starts functioning again.
> >>>>
> >>>> The 30s "hang" is the default command timeout: your drive is not responding to
> >>>> the DMA version of READ LOG EXT command. There are some drives out there like
> >>>> that. So instead of completely disabling access to the log directory, we should
> >>>> simply force the use of READ LOG EXT. And for that, there is the horkage flag
> >>>> ATA_HORKAGE_NO_DMA_LOG.
> >>>>
> >>>> Can you try using that one without the patch I sent ?
> >>>
> >>> I think I tested that before submitting the patch, but re-tested it now
> >>> and that doesn't work:
> >>
> >> Your original patch used the ATA_HORKAGE_NO_ID_DEV_LOG flag. What
> >> ATA_HORKAGE_NO_DMA_LOG does is prevent the use of the READ LOG DMA EXT
> >> command. READ LOG EXT command (PIO) will be used. And the sg commands I
> >> sent you and that you tried show that it works:
> >>
> >> sg_sat_read_gplog --log=0 --page=0 --readonly
> >>
> >> worked perfectly, while:
> >>
> >> sg_sat_read_gplog --dma --log=0 --page=0 --readonly
> >>
> >> failed.
> >>
> >> So by simply preventing the use of READ LOG DMA EXT for your drive, the
> >> kernel will be able to safely get the log directory and test for other
> >> page support.
> >>
> >> In your original patch, try replacing ATA_HORKAGE_NO_ID_DEV_LOG with
> >> ATA_HORKAGE_NO_DMA_LOG.
> >
> > I could have bin clearer. This log snippet below is a 5.15.10 with
> > ATA_HORKAGE_NO_DMA_LOG set for this device.
> >
> > Shure, sg_sat_read_gplog --log=0 --page=0 --readonly worked when booted,
> > but as it looks to me, it doesn't work during boot.
>
> Very weird... So I guess the big-hammer patch introducing
> ATA_HORKAGE_NO_LOG_DIR seems like the best solution for your drive.
> Please review and test again the patch I sent and post it to linux-ide
> for review.
Indeed. My guess would be that it depends on where in the init process
the drive is how it behaves.
Patch is re-tested and re-submitted. Thanks for all your help figuring
this out and getting this on the way upstream.
//Anton
