Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 16, 2021 at 12:10:28AM +0000, Damien Le Moal wrote:
> > +	/* Do not even attempt DMA with PATA-SATA adapters, they seem likely to
> > +	 * hang, see https://bugzilla.kernel.org/show_bug.cgi?id=195895
> > +	 */
> 
> This is not the standard kernel comment style. Multi-lines comment should start
> with a "/*" line:

Fixed, somehow I thought I had followed the surrounding style, I was
wrong.

> Also, I would remove the bugzilla reference since it is in the commit message.

Done, though long-term the commit message might be harder to find than
the comment...

> >  	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
> > +	    (ap_flags & ATA_FLAG_SATA) &&
> 
> Why is this necessary since you have the ATA_HORKAGE_NO_DMA_LOG flag ?

Not sure I understand the question right.
The patch is a best-guess + "in doubt disable" attempt at
auto-detecting.
As I mentioned the data is rather light, so I wanted to only add the
option at first.
Now it does 2 things in addition:
- disable it for Crucial MX500 because the issue was seen and confirmed
  with and without PATA converter with that device
- disable it for anything connected with a PATA converter because there
  are lots of bug reports with different devices that have a PATA
  converter in common. Unfortunately no confirmation from any of those.
  However in quick testing I could verify the MX500 issue only with
  PATA adapter myself, so it seems like an appropriate precaution,
  especially as now with the new code it is possible to override both
  ways.

(should I put something more in the commit message? I didn't want to
bloat it too much)

Aside: I don't have the ATA spec, so I can't check, but I do find it
a bit suspicious that the logs look like the code runs the
READ_LOG_DMA command before SETXFERMODE is done, and thus while the
device is still in PIO mode?

> If this is really necessary, ATA_HORKAGE_NO_DMA_LOG should be set for all
> devices that have ATA_FLAG_SATA when the device is initialized. With that, this
> additional condition can go away.

I missed the right place to do that before, I think I now found it, so
moved.

Thanks for the review,
Reimar



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux