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.
> Beware that if the log drectory is the problem, this will lockup your drive !
> If that is OK and works, then please try:
>
> sg_sat_read_gplog --dma --log=48 --page=0 --readonly
# sg_sat_read_gplog --dma --log=48 --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=48 --page=0 --readonly
sg_sat_read_gplog --log=48 --page=0 --readonly /dev/sda
ATA PASS-THROUGH (16), bad field in cdb
sg_sat_read_gplog failed: Illegal request
> log=48 (0x30) is the identify device log page. log=0 is the log directory log page.
So, does this means that it might be where in the init of the device the
directory log page is accessed that causes the device to fault?
A snippet from a failed boot before the patch looks like:
ata1.00: qc timeout (cmd 0x2f)
ata1.00: Read log 0x00 page 0x00 failed, Emask 0x4
ata1.00: ATA Identify Device Log not supported
ata1.00: failed to set xfermode (err_mask=0x40)
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: limiting SATA link speed to 3.0 Gbps
ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
ata1.00: qc timeout (cmd 0x2f)
ata1.00: Read log 0x00 page 0x00 failed, Emask 0x4
ata1.00: ATA Identify Device Log not supported
ata1.00: failed to set xfermode (err_mask=0x40)
ata1.00: disabled
ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
When I messed around trying to figure out what the fault was I saw a
couple of different Emask, 0x4, 0x40 and 0x80.
I also blindly played around with adding a ata_msleep(ap, 500); after
the failed attempt to access the log directory log page, but that didn't
help.
//Anton
