Re: [PATCH v2] ahci: libahci: clear pending interrupt status

[Niklas Cassel]

Hello Szuying Chen,

Code wise, I think this looks good, it just needs some improvements
to meet the standard for kernel patches.

On Fri, Sep 01, 2023 at 04:36:13PM +0800, Szuying Chen wrote:
> This patch adds the function to clear pending interrupt before COMRESET.
> It follows the  AHCI1.3.1 - section6.2.2.2 specification.

No need for double spaces between "the  AHCI1.3.1".

Also:
s/AHCI1.3.1/AHCI 1.3.1/
s/section6.2.2.2/section 6.2.2.2/


Damien asked you to describe your changes better:
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

The commit log should first describe the problem,
then it should describe how the problem is fixed.


> 
> Signed-off-by: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
> ---modify function name and substitution related behavior.

This does not look correct, I think you accidentally removed the newline
after "---".

The patch format is:
https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format

So this should have been something like:

Signed-off-by: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
---
V1 -> V2: Renamed helper function and modified ahci_port_init() to make
use of the helper.


> 
>  drivers/ata/libahci.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 06aec35f88f2..f8ecd9956ea1 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1256,6 +1256,27 @@ static ssize_t ahci_activity_show(struct ata_device *dev, char *buf)
>  	return sprintf(buf, "%d\n", emp->blink_policy);
>  }
> 
> +static void ahci_port_clear_pending_irq(struct ata_port *ap)
> +{
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	u32 tmp;
> +
> +	/* clear SError */
> +	tmp = readl(port_mmio + PORT_SCR_ERR);
> +	dev_dbg(ap->host->dev, "PORT_SCR_ERR 0x%x\n", tmp);
> +	writel(tmp, port_mmio + PORT_SCR_ERR);
> +
> +	/* clear port IRQ */
> +	tmp = readl(port_mmio + PORT_IRQ_STAT);
> +	dev_dbg(ap->host->dev, "PORT_IRQ_STAT 0x%x\n", tmp);
> +	if (tmp)
> +		writel(tmp, port_mmio + PORT_IRQ_STAT);
> +
> +	writel(1 << ap->port_no, hpriv->mmio + HOST_IRQ_STAT);
> +

Remove this extra newline.


> +}
> +
>  static void ahci_port_init(struct device *dev, struct ata_port *ap,
>  			   int port_no, void __iomem *mmio,
>  			   void __iomem *port_mmio)
> @@ -1270,18 +1291,8 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
>  	if (rc)
>  		dev_warn(dev, "%s (%d)\n", emsg, rc);
> 
> -	/* clear SError */
> -	tmp = readl(port_mmio + PORT_SCR_ERR);
> -	dev_dbg(dev, "PORT_SCR_ERR 0x%x\n", tmp);
> -	writel(tmp, port_mmio + PORT_SCR_ERR);
> -
> -	/* clear port IRQ */
> -	tmp = readl(port_mmio + PORT_IRQ_STAT);
> -	dev_dbg(dev, "PORT_IRQ_STAT 0x%x\n", tmp);
> -	if (tmp)
> -		writel(tmp, port_mmio + PORT_IRQ_STAT);
> -
> -	writel(1 << port_no, mmio + HOST_IRQ_STAT);
> +	/* clear pending irq */
> +	ahci_port_clear_pending_irq(ap);
> 
>  	/* mark esata ports */
>  	tmp = readl(port_mmio + PORT_CMD);
> @@ -1602,6 +1613,9 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>  	tf.status = ATA_BUSY;
>  	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
> 
> +	/* clear pending irq */

Personally, I'm not a fan of this comment.
The function name is already so descriptive that the comment seems
superfluous.


> +	ahci_port_clear_pending_irq(ap);
> +
>  	rc = sata_link_hardreset(link, timing, deadline, online,
>  				 ahci_check_ready);
> 
> --
> 2.39.2
>