linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: "Reimar Döffinger" <Reimar.Doeffinger@gmx.de>
Cc: "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
	Jens Axboe <axboe@kernel.dk>,
	"hch@infradead.org" <hch@infradead.org>
Subject: Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.
Date: Tue, 17 Aug 2021 07:45:58 +0000	[thread overview]
Message-ID: <DM6PR04MB708170BD5DC19ABC03FDC4A3E7FE9@DM6PR04MB7081.namprd04.prod.outlook.com> (raw)
In-Reply-To: <YRqfxrWJsIeyjFed@reimardoeffinger.de>

On 2021/08/17 2:26, Reimar Döffinger wrote:
> 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...

Yes, but long-term, web links tend to go stale too :)

> 
>>>  	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.

I was referring to the added condition "&& (ap_flags & ATA_FLAG_SATA)".
If the port has this flag, then setting the ATA_HORKAGE_NO_DMA_LOG for the
device before coming here would not require this additional condition.

> 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.

How can you detect the presence of a PATA converter ? Is it what "ap_flags &
ATA_FLAG_SATA" indicates ?

> 
> (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?

ata_read_log_page() has a retry loop: it first tries ATA_CMD_READ_LOG_DMA_EXT
and if that fails, it goes with PIO ATA_CMD_READ_LOG_EXT. So there is no problem
for non-buggy drives with this. Note that initially, dev->dma_mode may not be
set, so PIO is done directly. Once set, DMA version will be used unless the
horkage bit is set. So this all works as expected, and there will (normally) be
no failed command even when DMA is not yet set.

> 
>> 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
> 


-- 
Damien Le Moal
Western Digital Research

  reply	other threads:[~2021-08-17  7:46 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14 13:47 [PATCH] libata: add kernel parameter to force ATA_HORKAGE_NO_DMA_LOG Reimar Döffinger
2021-08-15  8:18 ` Christoph Hellwig
2021-08-15 16:27   ` [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases Reimar Döffinger
2021-08-16  0:10     ` Damien Le Moal
2021-08-16 17:15       ` Reimar Döffinger
2021-08-17 19:06         ` Reimar Döffinger
2021-08-18 22:49         ` Damien Le Moal
2021-08-16 17:26       ` Reimar Döffinger
2021-08-17  7:45         ` Damien Le Moal [this message]
2021-08-17 18:01           ` Reimar Döffinger
2021-08-17 19:03             ` [PATCH] libata: fix setting and checking of DMA state Reimar Döffinger
2021-08-18 23:30               ` Damien Le Moal
2021-08-19  8:13                 ` [PATCH] libata: fix " Reimar Döffinger
2021-08-19 12:56                   ` Reimar Döffinger
2021-09-27  8:56                   ` Paul Menzel
2021-09-27  9:10                     ` Reimar Döffinger
2021-09-27  9:15                       ` Damien Le Moal
2021-10-03 13:28                         ` [PATCH v4] Fixes to DMA state check Reimar Döffinger
2021-10-03 13:28                           ` [PATCH 1/6] libata: fix checking of DMA state Reimar Döffinger
2021-10-03 13:28                           ` [PATCH 2/6] libata-scsi: " Reimar Döffinger
2021-10-03 13:28                           ` [PATCH 3/6] pata_ali: " Reimar Döffinger
2021-10-03 13:28                           ` [PATCH 4/6] pata_amd: " Reimar Döffinger
2021-10-03 13:28                           ` [PATCH 5/6] pata_optidma: " Reimar Döffinger
2021-10-03 13:28                           ` [PATCH 6/6] pata_radisys: " Reimar Döffinger
2021-10-12  2:20                           ` [PATCH v4] Fixes to DMA state check Damien Le Moal
2021-10-12  6:30                             ` Reimar Döffinger
     [not found]                             ` <605EE991-5DA2-4A8D-9691-967D68D3AB51@gmx.de>
2021-10-12  6:32                               ` Damien Le Moal
2021-10-12  6:33                               ` Damien Le Moal
2021-08-15 16:34   ` [PATCH] libata: add kernel parameter to force ATA_HORKAGE_NO_DMA_LOG Reimar Döffinger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR04MB708170BD5DC19ABC03FDC4A3E7FE9@DM6PR04MB7081.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=Reimar.Doeffinger@gmx.de \
    --cc=axboe@kernel.dk \
    --cc=hch@infradead.org \
    --cc=linux-ide@vger.kernel.org \
    --subject='Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).