Re: [PATCH v3 3/4] ata: ahci_imx: Enlarge RX water mark for i.MX8QM SATA

[Damien Le Moal <dlemoal@kernel.org>]

On 7/18/24 3:17 AM, Niklas Cassel wrote:
> Hello Richard,
> 
> On Tue, Jul 16, 2024 at 11:18:14AM +0800, Richard Zhu wrote:
>> The RXWM(RxWaterMark) sets the minimum number of free location within
>> the RX FIFO before the watermark is exceeded which in turn will cause
>> the Transport Layer to instruct the Link Layer to transmit HOLDS to
>> the transmitting end.
>>
>> Based on the default RXWM value 0x20, RX FIFO overflow might be
>> observed on i.MX8QM MEK board, when some Gen3 SATA disks are used.
>>
>> The FIFO overflow will result in CRC error, internal error and protocol
>> error, then the SATA link is not stable anymore.
>>
>> To fix this issue, enlarge RX water mark setting from 0x20 to 0x29.
>>
>> Fixes: 027fa4dee935 ("ahci: imx: add the imx8qm ahci sata support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
>> ---
> 
> Looking at the title of this patch:
> "ahci_imx: Enlarge RX water mark for i.MX8QM SATA"
> 
> This suggests that this fix is only needed for i.MX8QM.
> 
> Support for i.MX8QM was added to the device tree binding in patch 1/4 in
> this series.
> 
> Doing a git grep in linux-next gives the following result:
> 
> $ git grep fsl,imx8qm-ahci linux-next/master
> linux-next/master:drivers/ata/ahci_imx.c:       { .compatible = "fsl,imx8qm-ahci", .data = (void *)AHCI_IMX8QM },
> 
> 
> This is interesting for two reasons:
> 1) drivers/ata/ahci_imx.c already has support for this compatible string,
> even though this compatible string does not exist in any DT binding
> (in linux-next).
> 
> 2) There is not a single in-tree device tree (DTS) that uses this compatible
> string ....and we do not care about out of tree device trees.
> 
> 
> Considering 2) I do NOT think that we should have
> Cc: stable@vger.kernel.org on this... we shouldn't just backport random driver
> fixes is there are no in-tree users of this compatible string.
> 
> So I suggest that:
> -Drop the CC: stable.
> -I actually think that it is better that you drop the Fixes tag too, because if
> you keep it, the stable bots will automatically select this for backporting,
> and then we will need to reply and say that this should not be backported, so
> better to avoid adding the Fixes tag in the first place.
> (Since there are no users of this compatible string, there is nothing that is
> broken, so there is nothing to fix.)
> 
> 
> Damien, when applying this patch, I suggest that we apply it to for-6.12
> together with the rest of the series (instead of applying it to
> for-6.11-fixes).

It was me who asked for the Fixes and Cc-stable tags, but I had not checked
that the compatible is not being used in any DT. So good catch.
I will apply everything to for-6.12.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research