[PATCH 0/2] Refactor the accessors for the ATA device control and alternate status registers

[PATCH 0/2] Refactor the accessors for the ATA device control and alternate status registers

[Sergey Shtylyov]

Here are 2 patches against the 'for-next' branch of Damien Le Moal's
'libata.git' repo. Refactor the accessors for the device control and
alternate status registers moving the prerequeiste checks done now around
the calls into the functionas themselves and make them return a 'bool'
result indicating if a respective register exists or not...

Sergey Shtylyov (2):
  ata: libata-sff: refactor ata_sff_set_devctl()
  ata: libata-sff: refactor ata_sff_altstatus()

 drivers/ata/libata-sff.c | 86 +++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 40 deletions(-)

-- 
2.26.3

[PATCH v2 1/2] ata: libata-sff: refactor ata_sff_set_devctl()

[Sergey Shtylyov]

Commit 41dec29bcb05 ("libata: introduce sff_set_devctl() method") left some
clumsy checks surrounding calls to ata_sff_set_devctl() which Jeff Garzik
suggested to factor out...  and I never followed up. :-(

At last, refactor ata_sff_set_devctl() to include the repetitive checks and
return a 'bool' result indicating if the device control register exists or
not.

While at it, further update the 'kernel-doc' comment -- the device control
register has never been a part of the taskfile, despite what Jeff and co.
think! :-)

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/libata-sff.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 546b1f73ede5..3fb5bd4de50c 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -265,20 +265,26 @@ EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
  *	@ap: port where the device is
  *	@ctl: value to write
  *
- *	Writes ATA taskfile device control register.
+ *	Writes ATA device control register.
  *
- *	Note: may NOT be used as the sff_set_devctl() entry in
- *	ata_port_operations.
+ *	RETURN:
+ *	true if the register exists, false if not.
  *
  *	LOCKING:
  *	Inherited from caller.
  */
-static void ata_sff_set_devctl(struct ata_port *ap, u8 ctl)
+static bool ata_sff_set_devctl(struct ata_port *ap, u8 ctl)
 {
-	if (ap->ops->sff_set_devctl)
+	if (ap->ops->sff_set_devctl) {
 		ap->ops->sff_set_devctl(ap, ctl);
-	else
+		return true;
+	}
+	if (ap->ioaddr.ctl_addr) {
 		iowrite8(ctl, ap->ioaddr.ctl_addr);
+		return true;
+	}
+
+	return false;
 }
 
 /**
@@ -357,8 +363,6 @@ static void ata_dev_select(struct ata_port *ap, unsigned int device,
  */
 void ata_sff_irq_on(struct ata_port *ap)
 {
-	struct ata_ioports *ioaddr = &ap->ioaddr;
-
 	if (ap->ops->sff_irq_on) {
 		ap->ops->sff_irq_on(ap);
 		return;
@@ -367,8 +371,7 @@ void ata_sff_irq_on(struct ata_port *ap)
 	ap->ctl &= ~ATA_NIEN;
 	ap->last_ctl = ap->ctl;
 
-	if (ap->ops->sff_set_devctl || ioaddr->ctl_addr)
-		ata_sff_set_devctl(ap, ap->ctl);
+	ata_sff_set_devctl(ap, ap->ctl);
 	ata_wait_idle(ap);
 
 	if (ap->ops->sff_irq_clear)
@@ -1662,8 +1665,7 @@ void ata_sff_freeze(struct ata_port *ap)
 	ap->ctl |= ATA_NIEN;
 	ap->last_ctl = ap->ctl;
 
-	if (ap->ops->sff_set_devctl || ap->ioaddr.ctl_addr)
-		ata_sff_set_devctl(ap, ap->ctl);
+	ata_sff_set_devctl(ap, ap->ctl);
 
 	/* Under certain circumstances, some controllers raise IRQ on
 	 * ATA_NIEN manipulation.  Also, many controllers fail to mask
@@ -2061,10 +2063,8 @@ void ata_sff_postreset(struct ata_link *link, unsigned int *classes)
 		return;
 
 	/* set up device control */
-	if (ap->ops->sff_set_devctl || ap->ioaddr.ctl_addr) {
-		ata_sff_set_devctl(ap, ap->ctl);
+	if (ata_sff_set_devctl(ap, ap->ctl))
 		ap->last_ctl = ap->ctl;
-	}
 }
 EXPORT_SYMBOL_GPL(ata_sff_postreset);
 
-- 
2.26.3

Re: [PATCH v2 1/2] ata: libata-sff: refactor ata_sff_set_devctl()

[Damien Le Moal]

On 2022/02/14 0:10, Sergey Shtylyov wrote:
> Commit 41dec29bcb05 ("libata: introduce sff_set_devctl() method") left some
> clumsy checks surrounding calls to ata_sff_set_devctl() which Jeff Garzik
> suggested to factor out...  and I never followed up. :-(
> 
> At last, refactor ata_sff_set_devctl() to include the repetitive checks and
> return a 'bool' result indicating if the device control register exists or
> not.
> 
> While at it, further update the 'kernel-doc' comment -- the device control
> register has never been a part of the taskfile, despite what Jeff and co.
> think! :-)
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

This patch has "v2"... I do not recall seeing a v1 and the cover letter has no
changelog (and no v2 tag)... Is this a new series ?

> ---
>  drivers/ata/libata-sff.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 546b1f73ede5..3fb5bd4de50c 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -265,20 +265,26 @@ EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
>   *	@ap: port where the device is
>   *	@ctl: value to write
>   *
> - *	Writes ATA taskfile device control register.
> + *	Writes ATA device control register.
>   *
> - *	Note: may NOT be used as the sff_set_devctl() entry in
> - *	ata_port_operations.
> + *	RETURN:
> + *	true if the register exists, false if not.
>   *
>   *	LOCKING:
>   *	Inherited from caller.
>   */
> -static void ata_sff_set_devctl(struct ata_port *ap, u8 ctl)
> +static bool ata_sff_set_devctl(struct ata_port *ap, u8 ctl)
>  {
> -	if (ap->ops->sff_set_devctl)
> +	if (ap->ops->sff_set_devctl) {
>  		ap->ops->sff_set_devctl(ap, ctl);
> -	else
> +		return true;
> +	}
> +	if (ap->ioaddr.ctl_addr) {
>  		iowrite8(ctl, ap->ioaddr.ctl_addr);
> +		return true;
> +	}
> +
> +	return false;
>  }
>  
>  /**
> @@ -357,8 +363,6 @@ static void ata_dev_select(struct ata_port *ap, unsigned int device,
>   */
>  void ata_sff_irq_on(struct ata_port *ap)
>  {
> -	struct ata_ioports *ioaddr = &ap->ioaddr;
> -
>  	if (ap->ops->sff_irq_on) {
>  		ap->ops->sff_irq_on(ap);
>  		return;
> @@ -367,8 +371,7 @@ void ata_sff_irq_on(struct ata_port *ap)
>  	ap->ctl &= ~ATA_NIEN;
>  	ap->last_ctl = ap->ctl;
>  
> -	if (ap->ops->sff_set_devctl || ioaddr->ctl_addr)
> -		ata_sff_set_devctl(ap, ap->ctl);
> +	ata_sff_set_devctl(ap, ap->ctl);
>  	ata_wait_idle(ap);
>  
>  	if (ap->ops->sff_irq_clear)
> @@ -1662,8 +1665,7 @@ void ata_sff_freeze(struct ata_port *ap)
>  	ap->ctl |= ATA_NIEN;
>  	ap->last_ctl = ap->ctl;
>  
> -	if (ap->ops->sff_set_devctl || ap->ioaddr.ctl_addr)
> -		ata_sff_set_devctl(ap, ap->ctl);
> +	ata_sff_set_devctl(ap, ap->ctl);
>  
>  	/* Under certain circumstances, some controllers raise IRQ on
>  	 * ATA_NIEN manipulation.  Also, many controllers fail to mask
> @@ -2061,10 +2063,8 @@ void ata_sff_postreset(struct ata_link *link, unsigned int *classes)
>  		return;
>  
>  	/* set up device control */
> -	if (ap->ops->sff_set_devctl || ap->ioaddr.ctl_addr) {
> -		ata_sff_set_devctl(ap, ap->ctl);
> +	if (ata_sff_set_devctl(ap, ap->ctl))
>  		ap->last_ctl = ap->ctl;
> -	}
>  }
>  EXPORT_SYMBOL_GPL(ata_sff_postreset);
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-sff: refactor ata_sff_set_devctl()

[Sergey Shtylyov]

On 2/14/22 2:25 AM, Damien Le Moal wrote:

>> Commit 41dec29bcb05 ("libata: introduce sff_set_devctl() method") left some
>> clumsy checks surrounding calls to ata_sff_set_devctl() which Jeff Garzik
>> suggested to factor out...  and I never followed up. :-(
>>
>> At last, refactor ata_sff_set_devctl() to include the repetitive checks and
>> return a 'bool' result indicating if the device control register exists or
>> not.
>>
>> While at it, further update the 'kernel-doc' comment -- the device control
>> register has never been a part of the taskfile, despite what Jeff and co.
>> think! :-)
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> This patch has "v2"... I do not recall seeing a v1 and the cover letter has no
> changelog (and no v2 tag)... Is this a new series ?

   That v2 is there by mistake, please ignore it.

[...]

MBR, Sergey

[PATCH v2 2/2] ata: libata-sff: refactor ata_sff_altstatus()

[Sergey Shtylyov]

The driver's calls to ata_sff_altstatus() are mostly surrounded by some
clumsy checks.  Refactor ata_sff_altstatus() to include the repetitive
checks and return a 'bool' result indicating if the alternate
status register exists or not.

While at it, further update the 'kernel-doc' comment -- the alternate
status register has never been a part of the taskfile, despite what
Jeff and co. think! :-)

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/libata-sff.c | 56 ++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3fb5bd4de50c..fc8915964919 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -70,22 +70,35 @@ EXPORT_SYMBOL_GPL(ata_sff_check_status);
 /**
  *	ata_sff_altstatus - Read device alternate status reg
  *	@ap: port where the device is
+ *	@status: pointer to a status value
  *
- *	Reads ATA taskfile alternate status register for
- *	currently-selected device and return its value.
+ *	Reads ATA alternate status register for currently-selected device
+ *	and return its value.
  *
- *	Note: may NOT be used as the check_altstatus() entry in
- *	ata_port_operations.
+ *	RETURN:
+ *	true if the register exists, false if not.
  *
  *	LOCKING:
  *	Inherited from caller.
  */
-static u8 ata_sff_altstatus(struct ata_port *ap)
+static bool ata_sff_altstatus(struct ata_port *ap, u8 *status)
 {
-	if (ap->ops->sff_check_altstatus)
-		return ap->ops->sff_check_altstatus(ap);
+	u8 tmp;
 
-	return ioread8(ap->ioaddr.altstatus_addr);
+	if (ap->ops->sff_check_altstatus) {
+		tmp = ap->ops->sff_check_altstatus(ap);
+		goto read;
+	}
+	if (ap->ioaddr.altstatus_addr) {
+		tmp = ioread8(ap->ioaddr.altstatus_addr);
+		goto read;
+	}
+	return false;
+
+read:
+	if (status)
+		*status = tmp;
+	return true;
 }
 
 /**
@@ -104,12 +117,9 @@ static u8 ata_sff_irq_status(struct ata_port *ap)
 {
 	u8 status;
 
-	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
-		status = ata_sff_altstatus(ap);
-		/* Not us: We are busy */
-		if (status & ATA_BUSY)
-			return status;
-	}
+	/* Not us: We are busy */
+	if (ata_sff_altstatus(ap, &status) && (status & ATA_BUSY))
+		return status;
 	/* Clear INTRQ latch */
 	status = ap->ops->sff_check_status(ap);
 	return status;
@@ -129,10 +139,7 @@ static u8 ata_sff_irq_status(struct ata_port *ap)
 
 static void ata_sff_sync(struct ata_port *ap)
 {
-	if (ap->ops->sff_check_altstatus)
-		ap->ops->sff_check_altstatus(ap);
-	else if (ap->ioaddr.altstatus_addr)
-		ioread8(ap->ioaddr.altstatus_addr);
+	ata_sff_altstatus(ap, NULL);
 }
 
 /**
@@ -164,12 +171,12 @@ EXPORT_SYMBOL_GPL(ata_sff_pause);
 
 void ata_sff_dma_pause(struct ata_port *ap)
 {
-	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
-		/* An altstatus read will cause the needed delay without
-		   messing up the IRQ status */
-		ata_sff_altstatus(ap);
+	/*
+	 * An altstatus read will cause the needed delay without
+	 * messing up the IRQ status
+	 */
+	if (ata_sff_altstatus(ap, NULL))
 		return;
-	}
 	/* There are no DMA controllers without ctl. BUG here to ensure
 	   we never violate the HDMA1:0 transition timing and risk
 	   corruption. */
@@ -1637,8 +1644,7 @@ void ata_sff_lost_interrupt(struct ata_port *ap)
 		return;
 	/* See if the controller thinks it is still busy - if so the command
 	   isn't a lost IRQ but is still in progress */
-	status = ata_sff_altstatus(ap);
-	if (status & ATA_BUSY)
+	if (ata_sff_altstatus(ap, &status) && (status & ATA_BUSY))
 		return;
 
 	/* There was a command running, we are no longer busy and we have
-- 
2.26.3

Re: [PATCH v2 2/2] ata: libata-sff: refactor ata_sff_altstatus()

[Dan Carpenter]

Hi Sergey,

url:    https://github.com/0day-ci/linux/commits/Sergey-Shtylyov/Refactor-the-accessors-for-the-ATA-device-control-and-alternate-status-registers/20220213-231119
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b81b1829e7e39f6cebdf6e4d5484eacbceda8554
config: riscv-randconfig-m031-20220213 (https://download.01.org/0day-ci/archive/20220214/202202141714.X7UdMJUj-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/ata/libata-sff.c:1652 ata_sff_lost_interrupt() error: uninitialized symbol 'status'.

vim +/status +1652 drivers/ata/libata-sff.c

c96f1732e25362 Alan Cox        2009-03-24  1635  void ata_sff_lost_interrupt(struct ata_port *ap)
c96f1732e25362 Alan Cox        2009-03-24  1636  {
c96f1732e25362 Alan Cox        2009-03-24  1637  	u8 status;
c96f1732e25362 Alan Cox        2009-03-24  1638  	struct ata_queued_cmd *qc;
c96f1732e25362 Alan Cox        2009-03-24  1639  
c96f1732e25362 Alan Cox        2009-03-24  1640  	/* Only one outstanding command per SFF channel */
c96f1732e25362 Alan Cox        2009-03-24  1641  	qc = ata_qc_from_tag(ap, ap->link.active_tag);
3e4ec3443f70fb Tejun Heo       2010-05-10  1642  	/* We cannot lose an interrupt on a non-existent or polled command */
3e4ec3443f70fb Tejun Heo       2010-05-10  1643  	if (!qc || qc->tf.flags & ATA_TFLAG_POLLING)
c96f1732e25362 Alan Cox        2009-03-24  1644  		return;
c96f1732e25362 Alan Cox        2009-03-24  1645  	/* See if the controller thinks it is still busy - if so the command
c96f1732e25362 Alan Cox        2009-03-24  1646  	   isn't a lost IRQ but is still in progress */
57232468aca7de Sergey Shtylyov 2022-02-13  1647  	if (ata_sff_altstatus(ap, &status) && (status & ATA_BUSY))

"status" is not intialized if ata_sff_altstatus() return false.

c96f1732e25362 Alan Cox        2009-03-24  1648  		return;
c96f1732e25362 Alan Cox        2009-03-24  1649  
c96f1732e25362 Alan Cox        2009-03-24  1650  	/* There was a command running, we are no longer busy and we have
c96f1732e25362 Alan Cox        2009-03-24  1651  	   no interrupt. */
a9a79dfec23956 Joe Perches     2011-04-15 @1652  	ata_port_warn(ap, "lost interrupt (Status 0x%x)\n",
c96f1732e25362 Alan Cox        2009-03-24  1653  								status);
                                                                                                                ^^^^^^
Uninitalized.  Also the indenting is unfortunate.

c96f1732e25362 Alan Cox        2009-03-24  1654  	/* Run the host interrupt logic as if the interrupt had not been
c96f1732e25362 Alan Cox        2009-03-24  1655  	   lost */
c3b2889424c26f Tejun Heo       2010-05-19  1656  	ata_sff_port_intr(ap, qc);
c96f1732e25362 Alan Cox        2009-03-24  1657  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 2/2] ata: libata-sff: refactor ata_sff_altstatus()

[Sergey Shtylyov]

On 2/17/22 3:12 PM, Dan Carpenter wrote:

> url:    https://github.com/0day-ci/linux/commits/Sergey-Shtylyov/Refactor-the-accessors-for-the-ATA-device-control-and-alternate-status-registers/20220213-231119
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b81b1829e7e39f6cebdf6e4d5484eacbceda8554
> config: riscv-randconfig-m031-20220213 (https://download.01.org/0day-ci/archive/20220214/202202141714.X7UdMJUj-lkp@intel.com/config)
> compiler: riscv32-linux-gcc (GCC) 11.2.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/ata/libata-sff.c:1652 ata_sff_lost_interrupt() error: uninitialized symbol 'status'.
> 
> vim +/status +1652 drivers/ata/libata-sff.c
> 
> c96f1732e25362 Alan Cox        2009-03-24  1635  void ata_sff_lost_interrupt(struct ata_port *ap)
> c96f1732e25362 Alan Cox        2009-03-24  1636  {
> c96f1732e25362 Alan Cox        2009-03-24  1637  	u8 status;
> c96f1732e25362 Alan Cox        2009-03-24  1638  	struct ata_queued_cmd *qc;
> c96f1732e25362 Alan Cox        2009-03-24  1639  
> c96f1732e25362 Alan Cox        2009-03-24  1640  	/* Only one outstanding command per SFF channel */
> c96f1732e25362 Alan Cox        2009-03-24  1641  	qc = ata_qc_from_tag(ap, ap->link.active_tag);
> 3e4ec3443f70fb Tejun Heo       2010-05-10  1642  	/* We cannot lose an interrupt on a non-existent or polled command */
> 3e4ec3443f70fb Tejun Heo       2010-05-10  1643  	if (!qc || qc->tf.flags & ATA_TFLAG_POLLING)
> c96f1732e25362 Alan Cox        2009-03-24  1644  		return;
> c96f1732e25362 Alan Cox        2009-03-24  1645  	/* See if the controller thinks it is still busy - if so the command
> c96f1732e25362 Alan Cox        2009-03-24  1646  	   isn't a lost IRQ but is still in progress */
> 57232468aca7de Sergey Shtylyov 2022-02-13  1647  	if (ata_sff_altstatus(ap, &status) && (status & ATA_BUSY))
> 
> "status" is not intialized if ata_sff_altstatus() return false.

   Hmm, inaccurate coding on my side, will fix it, thanks!

> c96f1732e25362 Alan Cox        2009-03-24  1648  		return;
> c96f1732e25362 Alan Cox        2009-03-24  1649  
> c96f1732e25362 Alan Cox        2009-03-24  1650  	/* There was a command running, we are no longer busy and we have
> c96f1732e25362 Alan Cox        2009-03-24  1651  	   no interrupt. */
> a9a79dfec23956 Joe Perches     2011-04-15 @1652  	ata_port_warn(ap, "lost interrupt (Status 0x%x)\n",
> c96f1732e25362 Alan Cox        2009-03-24  1653  								status);
>                                                                                                                 ^^^^^^
> Uninitalized.  Also the indenting is unfortunate.

   Not my fault but I'll fix this as well... :-)

[...]

MBR, Sergey

Re: [PATCH v2 2/2] ata: libata-sff: refactor ata_sff_altstatus()

[Damien Le Moal]

On 2/18/22 01:02, Sergey Shtylyov wrote:
> On 2/17/22 3:12 PM, Dan Carpenter wrote:
> 
>> url:    https://github.com/0day-ci/linux/commits/Sergey-Shtylyov/Refactor-the-accessors-for-the-ATA-device-control-and-alternate-status-registers/20220213-231119
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b81b1829e7e39f6cebdf6e4d5484eacbceda8554
>> config: riscv-randconfig-m031-20220213 (https://download.01.org/0day-ci/archive/20220214/202202141714.X7UdMJUj-lkp@intel.com/config)
>> compiler: riscv32-linux-gcc (GCC) 11.2.0
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> smatch warnings:
>> drivers/ata/libata-sff.c:1652 ata_sff_lost_interrupt() error: uninitialized symbol 'status'.
>>
>> vim +/status +1652 drivers/ata/libata-sff.c
>>
>> c96f1732e25362 Alan Cox        2009-03-24  1635  void ata_sff_lost_interrupt(struct ata_port *ap)
>> c96f1732e25362 Alan Cox        2009-03-24  1636  {
>> c96f1732e25362 Alan Cox        2009-03-24  1637  	u8 status;
>> c96f1732e25362 Alan Cox        2009-03-24  1638  	struct ata_queued_cmd *qc;
>> c96f1732e25362 Alan Cox        2009-03-24  1639  
>> c96f1732e25362 Alan Cox        2009-03-24  1640  	/* Only one outstanding command per SFF channel */
>> c96f1732e25362 Alan Cox        2009-03-24  1641  	qc = ata_qc_from_tag(ap, ap->link.active_tag);
>> 3e4ec3443f70fb Tejun Heo       2010-05-10  1642  	/* We cannot lose an interrupt on a non-existent or polled command */
>> 3e4ec3443f70fb Tejun Heo       2010-05-10  1643  	if (!qc || qc->tf.flags & ATA_TFLAG_POLLING)
>> c96f1732e25362 Alan Cox        2009-03-24  1644  		return;
>> c96f1732e25362 Alan Cox        2009-03-24  1645  	/* See if the controller thinks it is still busy - if so the command
>> c96f1732e25362 Alan Cox        2009-03-24  1646  	   isn't a lost IRQ but is still in progress */
>> 57232468aca7de Sergey Shtylyov 2022-02-13  1647  	if (ata_sff_altstatus(ap, &status) && (status & ATA_BUSY))
>>
>> "status" is not intialized if ata_sff_altstatus() return false.
> 
>    Hmm, inaccurate coding on my side, will fix it, thanks!
> 
>> c96f1732e25362 Alan Cox        2009-03-24  1648  		return;
>> c96f1732e25362 Alan Cox        2009-03-24  1649  
>> c96f1732e25362 Alan Cox        2009-03-24  1650  	/* There was a command running, we are no longer busy and we have
>> c96f1732e25362 Alan Cox        2009-03-24  1651  	   no interrupt. */
>> a9a79dfec23956 Joe Perches     2011-04-15 @1652  	ata_port_warn(ap, "lost interrupt (Status 0x%x)\n",
>> c96f1732e25362 Alan Cox        2009-03-24  1653  								status);
>>                                                                                                                 ^^^^^^
>> Uninitalized.  Also the indenting is unfortunate.
> 
>    Not my fault but I'll fix this as well... :-)

Did you send a fix ? I did not see anything in my inbox...

> 
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-sff: refactor ata_sff_altstatus()

[Damien Le Moal]

On 2/19/22 07:58, Damien Le Moal wrote:
> On 2/18/22 01:02, Sergey Shtylyov wrote:
>> On 2/17/22 3:12 PM, Dan Carpenter wrote:
>>
>>> url:    https://github.com/0day-ci/linux/commits/Sergey-Shtylyov/Refactor-the-accessors-for-the-ATA-device-control-and-alternate-status-registers/20220213-231119
>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b81b1829e7e39f6cebdf6e4d5484eacbceda8554
>>> config: riscv-randconfig-m031-20220213 (https://download.01.org/0day-ci/archive/20220214/202202141714.X7UdMJUj-lkp@intel.com/config)
>>> compiler: riscv32-linux-gcc (GCC) 11.2.0
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> smatch warnings:
>>> drivers/ata/libata-sff.c:1652 ata_sff_lost_interrupt() error: uninitialized symbol 'status'.
>>>
>>> vim +/status +1652 drivers/ata/libata-sff.c
>>>
>>> c96f1732e25362 Alan Cox        2009-03-24  1635  void ata_sff_lost_interrupt(struct ata_port *ap)
>>> c96f1732e25362 Alan Cox        2009-03-24  1636  {
>>> c96f1732e25362 Alan Cox        2009-03-24  1637  	u8 status;
>>> c96f1732e25362 Alan Cox        2009-03-24  1638  	struct ata_queued_cmd *qc;
>>> c96f1732e25362 Alan Cox        2009-03-24  1639  
>>> c96f1732e25362 Alan Cox        2009-03-24  1640  	/* Only one outstanding command per SFF channel */
>>> c96f1732e25362 Alan Cox        2009-03-24  1641  	qc = ata_qc_from_tag(ap, ap->link.active_tag);
>>> 3e4ec3443f70fb Tejun Heo       2010-05-10  1642  	/* We cannot lose an interrupt on a non-existent or polled command */
>>> 3e4ec3443f70fb Tejun Heo       2010-05-10  1643  	if (!qc || qc->tf.flags & ATA_TFLAG_POLLING)
>>> c96f1732e25362 Alan Cox        2009-03-24  1644  		return;
>>> c96f1732e25362 Alan Cox        2009-03-24  1645  	/* See if the controller thinks it is still busy - if so the command
>>> c96f1732e25362 Alan Cox        2009-03-24  1646  	   isn't a lost IRQ but is still in progress */
>>> 57232468aca7de Sergey Shtylyov 2022-02-13  1647  	if (ata_sff_altstatus(ap, &status) && (status & ATA_BUSY))
>>>
>>> "status" is not intialized if ata_sff_altstatus() return false.
>>
>>    Hmm, inaccurate coding on my side, will fix it, thanks!
>>
>>> c96f1732e25362 Alan Cox        2009-03-24  1648  		return;
>>> c96f1732e25362 Alan Cox        2009-03-24  1649  
>>> c96f1732e25362 Alan Cox        2009-03-24  1650  	/* There was a command running, we are no longer busy and we have
>>> c96f1732e25362 Alan Cox        2009-03-24  1651  	   no interrupt. */
>>> a9a79dfec23956 Joe Perches     2011-04-15 @1652  	ata_port_warn(ap, "lost interrupt (Status 0x%x)\n",
>>> c96f1732e25362 Alan Cox        2009-03-24  1653  								status);
>>>                                                                                                                 ^^^^^^
>>> Uninitalized.  Also the indenting is unfortunate.
>>
>>    Not my fault but I'll fix this as well... :-)
> 
> Did you send a fix ? I did not see anything in my inbox...

Ignore. Got it :)

> 
>>
>> [...]
>>
>> MBR, Sergey
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] Refactor the accessors for the ATA device control and alternate status registers

[Damien Le Moal]

On 2/14/22 00:10, Sergey Shtylyov wrote:
> Here are 2 patches against the 'for-next' branch of Damien Le Moal's
> 'libata.git' repo. Refactor the accessors for the device control and
> alternate status registers moving the prerequeiste checks done now around
> the calls into the functionas themselves and make them return a 'bool'
> result indicating if a respective register exists or not...
> 
> Sergey Shtylyov (2):
>   ata: libata-sff: refactor ata_sff_set_devctl()
>   ata: libata-sff: refactor ata_sff_altstatus()
> 
>  drivers/ata/libata-sff.c | 86 +++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 40 deletions(-)
> 

Applied to for-5.18. Thanks !


-- 
Damien Le Moal
Western Digital Research