linux-kernel - Re: [PATCH] libata/ahci: quirk for JMB585/JMB582

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <04372579-4c43-efa1-6042-270f6ae919c2@opensource.wdc.com>
Date:   Thu, 8 Sep 2022 07:13:05 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     MD Lin <mdlin@...cron.com>, axboe@...nel.dk
Cc:     linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
        kevinliu@...cron.com, charonchen@...cron.com,
        corahuang@...cron.com, mhchen@...cron.com, georgechao@...cron.com,
        banks@...cron.com, tzuwei@...cron.com
Subject: Re: [PATCH] libata/ahci: quirk for JMB585/JMB582

On 9/7/22 19:51, MD Lin wrote:
> This patch adds a quirk, which enable error bit handling functions
> and SATA configuration for JMicron JMB585/JMB582.
> 
> Signed-off-by: MD Lin <mdlin@...cron.com>

Please use scripts/get_maintainer.pl to check to whom ata patches should
be addressed. If you do not send patches to me, there is a high chance
that I will miss them.

The patch title should be:

ata: ahci: Add initialization quirk for Jmicro 585/582 controllers

Or equivalent, that is, a little more descriptive.

The commit message also does not explain WHY this quirk is necessary (the
problem is not described) and there is also no description of HOW your
patch address the issue. Please be a little more verbose with the commit
message to better describe the patch.

> ---
>  drivers/ata/ahci.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 505920d45..b0768fae3 100755
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1657,6 +1657,68 @@ static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hp
>  	}
>  }
>  
> +static void ahci_jmb585_write_sata_phy(void __iomem *mmio, u32 addr, u32 data)

The patch title said jmb585 and jmb582. So should this be called
ahci_jmb58x_write_sata_phy() ?

> +{
> +	writel((addr & 0x01FFFUL) + (1UL << 18UL), mmio + 0xC0);
> +	writel(data, mmio + 0xC8);
> +}
> +
> +static void ahci_jmicron_585_quirk(void __iomem *mmio)

Same here. Also, please be consistent with the names: spell out jmicron or
use jmb, either is fine with me, but once you choose one, stick with it.
So:

ahci_jmb58x_write_sata_phy()
ahci_jmb58x_quirk()

or

ahci_jmicron_58x_write_sata_phy()
ahci_jmicron_58x_quirk()

> +{
> +	u32 pi = readl(mmio + HOST_PORTS_IMPL);
> +	u32 b8_data;
> +
> +	/*
> +	 * enable error bit handling functions, these might overwrite
> +	 * the setting which loads from external SPI flash
> +	 */
> +	b8_data = (pi > 3) ? 0x13 : 0x92;
> +	writel(0x03060004+b8_data, mmio + 0xB8);

Spaces missing around the "+" in the first argument.

> +	writel(0x00FF0B01,         mmio + 0x30);
> +	writel(0x0000003F,         mmio + 0x34);
> +	writel(0x0000001F,         mmio + 0x38);
> +	writel(0x03060000+b8_data, mmio + 0xB8);

Same here.

> +	writel(0xF9E4EFBF,         mmio + 0xB0);

And what are all these magic values ? Where do they come from ?
It would be nice to have these defined as macros with descriptive names in
drivers/ata/ahci.h.

> +
> +	/*
> +	 * set SATA configuration, these might overwrite
> +	 * the setting which loads from external SPI flash
> +	 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x06, 0x70005BE3); /* port0 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x13, 0x70005BE3); /* port1 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x73, 0x000001E5); /* port0 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x75, 0x000001E5); /* port1 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x74, 0x00000024); /* port0 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x80, 0x250B0003); /* port1 */
> +	if (pi > 3) {
> +		ahci_jmb585_write_sata_phy(mmio, 0x20, 0x70005BE3); /* port2 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x2D, 0x70005BE3); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x3A, 0x70005BE3); /* port4 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x79, 0x000001E5); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x83, 0x250B0003); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x7A, 0x00000024); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x84, 0x250B0003); /* port3 */
> +	}

Same here, lots of "magic" values that cannot be checked. Please use
macros and add comments describing where these come from (adapter specs ?).

> +}
> +
> +static void ahci_jmicron_quirk(struct pci_dev *pdev,
> +			struct ahci_host_priv *hpriv)
> +{
> +	void __iomem *mmio = hpriv->mmio;
> +	u8 tmp8;
> +
> +	if (pdev->vendor != PCI_VENDOR_ID_JMICRON)
> +		return;
> +
> +	switch (pdev->device) {
> +	case 0x585: /* check if the chip is JMB585 */

The comment can be removed. This is obvious.

Also, there is no case for jmb582 model which the patch title mentions. Is
it the same number for both models ? If that is the case, then please add
a comment above the switch() describing that.

> +		tmp8 = readb(mmio + 0x44);
> +		if (tmp8)
> +			ahci_jmicron_585_quirk(mmio);

The tmp8 variable is not necessary.

> +		break;
> +	}
> +}
> +
>  static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	unsigned int board_id = ent->driver_data;
> @@ -1775,6 +1837,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 */
>  	ahci_intel_pcs_quirk(pdev, hpriv);
>  
> +	/* set JMicron configuration */

This is obvious from the function name. So you can drop this comment.

> +	ahci_jmicron_quirk(pdev, hpriv);
> +
>  	/* prepare host */
>  	if (hpriv->cap & HOST_CAP_NCQ) {
>  		pi.flags |= ATA_FLAG_NCQ;

-- 
Damien Le Moal
Western Digital Research