Re: [PATCH 2/6] rtc: abx80x: add irq to struct abx80x_priv

On Wed, Jul 22, 2026 at 08:13:53PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The shared IRQ handler unconditionally returns IRQ_HANDLED and inadvertently clears all status bits.
> - [High] The `abx80x_read_alarm` function fails unconditionally on success due to incorrect error checking.
> --
> 
> --- Patch [2]: [PATCH 2/6] rtc: abx80x: add irq to struct abx80x_priv ---
> commit ccef71986338cc749e8440b88d073968d465ea5c
> Author: Antoni Pokusinski <[email protected]>
> 
> rtc: abx80x: add irq to struct abx80x_priv
>     
> This patch moves the irq number into struct abx80x_priv so that it
> is no longer tied to the i2c_client. This allows the irq number to
> be accessed in the case of ABX81X SPI clocks.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > index 5486d9d0b1e5d..9b0e1d48d9ba3 100644
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> [ ... ]
> > @@ -285,11 +286,12 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> >  static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > +	struct abx80x_priv *priv = i2c_get_clientdata(client);
> >  	unsigned char buf[7];
> >  
> >  	int irq_mask, err;
> >  
> > -	if (client->irq <= 0)
> > +	if (priv->irq <= 0)
> >  		return -EINVAL;
> >  
> >  	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
> 
> [Severity: High]
> This is a pre-existing issue, but doesn't i2c_smbus_read_i2c_block_data() 
> return the number of bytes read on success?
> 
> Looking at drivers/rtc/rtc-abx80x.c:abx80x_read_alarm():
> 
> 	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
> 					    sizeof(buf), buf);
> 	if (err)
> 		return err;
> 
> Since it returns 7 on success, will this unconditionally fail and return
> early with an error code, leaving the alarm structure uninitialized?
> 
> [ ... ]
True but this is fixed in one of the subsequent patches where the
regmap usage is introduced.

> > @@ -921,19 +925,19 @@ static int abx80x_probe(struct i2c_client *client)
> >  	if (err)
> >  		return err;
> >  
> > -	if (client->irq > 0) {
> > -		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
> > -		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> > +	if (priv->irq > 0) {
> > +		dev_info(&client->dev, "IRQ %d supplied\n", priv->irq);
> > +		err = devm_request_threaded_irq(&client->dev, priv->irq, NULL,
> >  						abx80x_handle_irq,
> >  						IRQF_SHARED | IRQF_ONESHOT,
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but since the IRQ is registered
> here with IRQF_SHARED, does abx80x_handle_irq() correctly handle shared
> interrupts?
> 
> Looking at drivers/rtc/rtc-abx80x.c:abx80x_handle_irq():
> 
> static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> {
> 	[ ... ]
> 	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> 	if (status < 0)
> 		return IRQ_NONE;
> 	[ ... ]
> 	i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, 0);
> 
> 	return IRQ_HANDLED;
> }
> 
> If no relevant interrupt flags are set for this device, it still returns
> IRQ_HANDLED and writes 0 to ABX8XX_REG_STATUS, clearing all status bits
> including non-IRQ bits like BLF. Can this break other devices sharing the
> interrupt line and silently drop hardware status flags?
> 
Yeah so this looks like a genuine bug and should be fixed in a separate
patch I guess. In REG_STATUS we should only clear the bits AF or WDT if
they were set.

> >  						"abx8xx",
> >  						client);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2