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 >
