Re: [PATCH] libata: Don't issue ATA_LOG_DIRECTORY to SATADOM-ML 3ME

[Anton Lundin <glance@acc.umu.se>]

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@vger.kernel.org # v5.10+
> > Signed-off-by: Anton Lundin <glance@acc.umu.se>
> > 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.


//Anton