Re: [PATCH] i2c: tegra: proper handling of error cases

Re: [PATCH] i2c: tegra: proper handling of error cases

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6587 bytes --]

On Fri, Apr 15, 2016 at 06:51:47PM +0530, Shardar Shariff Md wrote:
> From: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> To summarize the issue observed in error cases:
> 
> In SW: For i2c message transfer, packet header
> and data payload is posted and then required
> error/packet completion interrupts are enabled
> later.
> 
> In HW flow: HW process the packet just after
> packet header is posted, if ARB lost/NACK error
> occurs (SW will not handle immediately when error
> happens as error interrupts are not enabled at
> this point). HW assumes error is acknowledged and
> clears current data in FIFO, But SW here posts the
> remaining data payload which still stays in FIFO
> as stale data (data without packet header).
> 
> Now once the interrupts are enabled, SW handles
> ARB lost/NACK error by clearing the ARB lost/NACK
> interrupt. Now HW assumes that SW attended the error
> and will parse/process stale data (data without packet
> header) present in FIFO which causes invalid NACK errors.
> 
> Fix: Enable the error interrupts before posting the
> packet into FIFO which make sure HW to not clear the
> fifo. Also disable the packet mode before acknowledging
> errors (ARB lost/NACK error) to not process any stale
> data.
> 
> As error interrupts are enabled before posting the
> packet header use spinlock to avoid preempting.

Please try to use up the full 78 characters per line in the commit
message (with the exception being the subject line that should be
shorter).

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
[...]
> @@ -423,12 +424,31 @@ static inline void tegra_i2c_clock_disable(struct tegra_i2c_dev *i2c_dev)
>  		clk_disable(i2c_dev->fast_clk);
>  }
>  
> +static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
> +{
> +	unsigned long timeout;
> +
> +	if (i2c_dev->hw->has_config_load_reg) {
> +		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> +		timeout = jiffies + msecs_to_jiffies(1000);
> +		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
> +			if (time_after(jiffies, timeout)) {
> +				dev_warn(i2c_dev->dev,
> +					"timeout waiting for config load\n");

Nit: since you're already changing this, perhaps make the arguments on
subsequent lines align with the first argument on the first line? Like
so:

				dev_warn(i2c_dev->dev,
					 "timeout waiting for config load\n");

Note the extra space used for padding.

> +				return -ETIMEDOUT;
> +			}
> +			msleep(1);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  {
>  	u32 val;
>  	int err = 0;
>  	u32 clk_divisor;
> -	unsigned long timeout = jiffies + HZ;
>  
>  	err = tegra_i2c_clock_enable(i2c_dev);
>  	if (err < 0) {
> @@ -477,20 +497,13 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  	if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
>  		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
>  
> -	if (i2c_dev->hw->has_config_load_reg) {
> -		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> -		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
> -			if (time_after(jiffies, timeout)) {
> -				dev_warn(i2c_dev->dev,
> -					"timeout waiting for config load\n");
> -				return -ETIMEDOUT;
> -			}
> -			msleep(1);
> -		}
> -	}
> +	err = tegra_i2c_wait_for_config_load(i2c_dev);
>  
>  	tegra_i2c_clock_disable(i2c_dev);
>  
> +	if (err)
> +		return err;
> +

The behaviour here is now different, though I assume it's really a fix
that disables the I2C clock in case the timeout happens. Given that I
suspect this would be better off in a separate patch (factor out the
tegra_i2c_wait_for_config_load() function and return error after the
clocks have been disabled).

>  	if (i2c_dev->irq_disabled) {
>  		i2c_dev->irq_disabled = 0;
>  		enable_irq(i2c_dev->irq);
> @@ -499,14 +512,28 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  	return err;
>  }
>  
> +static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 cnfg;
> +
> +	cnfg = i2c_readl(i2c_dev, I2C_CNFG);
> +	if (cnfg & I2C_CNFG_PACKET_MODE_EN)
> +		i2c_writel(i2c_dev, cnfg & (~I2C_CNFG_PACKET_MODE_EN),

Parentheses are not necessary here.

> +				I2C_CNFG);

If you drop the parentheses above this can be moved into the previous
line because it will fit within 80 columns.

> +
> +	return tegra_i2c_wait_for_config_load(i2c_dev);
> +}
> +
>  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  {
>  	u32 status;
>  	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
>  	struct tegra_i2c_dev *i2c_dev = dev_id;
> +	unsigned long flags;
>  
>  	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
>  
> +	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
>  	if (status == 0) {
>  		dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
>  			 i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
> @@ -522,6 +549,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	}
>  
>  	if (unlikely(status & status_err)) {
> +		tegra_i2c_disable_packet_mode(i2c_dev);

If you don't care about the return value it'd be better to reflect that
by making the function return void. I presume ignoring the error here is
safe?

> @@ -583,6 +614,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
>  	reinit_completion(&i2c_dev->msg_complete);
>  
> +	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
> +
> +	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> +	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> +
>  	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
>  			PACKET_HEADER0_PROTOCOL_I2C |
>  			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
> @@ -612,14 +648,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	if (!(msg->flags & I2C_M_RD))
>  		tegra_i2c_fill_tx_fifo(i2c_dev);
>  
> -	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
>  	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
>  		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
>  	if (msg->flags & I2C_M_RD)
>  		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
>  	else if (i2c_dev->msg_buf_remaining)
>  		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> +
>  	tegra_i2c_unmask_irq(i2c_dev, int_mask);

Can this line be removed? The NO_ACK and ARBITRATION_LOST interrupts are
already unmasked earlier.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] i2c: tegra: proper handling of error cases

[Shardar Mohammed]

Thanks for the review, updated with comments inline with Shardar.

> On Fri, Apr 15, 2016 at 06:51:47PM +0530, Shardar Shariff Md wrote:
> > From: Shardar Shariff Md <smohammed@nvidia.com>
> >
> > To summarize the issue observed in error cases:
> >
> > In SW: For i2c message transfer, packet header and data payload is
> > posted and then required error/packet completion interrupts are
> > enabled later.
> >
> > In HW flow: HW process the packet just after packet header is posted,
> > if ARB lost/NACK error occurs (SW will not handle immediately when
> > error happens as error interrupts are not enabled at this point). HW
> > assumes error is acknowledged and clears current data in FIFO, But SW
> > here posts the remaining data payload which still stays in FIFO as
> > stale data (data without packet header).
> >
> > Now once the interrupts are enabled, SW handles ARB lost/NACK error by
> > clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended
> > the error and will parse/process stale data (data without packet
> > header) present in FIFO which causes invalid NACK errors.
> >
> > Fix: Enable the error interrupts before posting the packet into FIFO
> > which make sure HW to not clear the fifo. Also disable the packet mode
> > before acknowledging errors (ARB lost/NACK error) to not process any
> > stale data.
> >
> > As error interrupts are enabled before posting the packet header use
> > spinlock to avoid preempting.
> 
> Please try to use up the full 78 characters per line in the commit message
> (with the exception being the subject line that should be shorter).

[Shardar] Will take care of this.

> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> [...]
> > @@ -423,12 +424,31 @@ static inline void tegra_i2c_clock_disable(struct
> tegra_i2c_dev *i2c_dev)
> >  		clk_disable(i2c_dev->fast_clk);
> >  }
> >
> > +static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev
> > +*i2c_dev) {
> > +	unsigned long timeout;
> > +
> > +	if (i2c_dev->hw->has_config_load_reg) {
> > +		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD,
> I2C_CONFIG_LOAD);
> > +		timeout = jiffies + msecs_to_jiffies(1000);
> > +		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
> > +			if (time_after(jiffies, timeout)) {
> > +				dev_warn(i2c_dev->dev,
> > +					"timeout waiting for config load\n");
> 
> Nit: since you're already changing this, perhaps make the arguments on
> subsequent lines align with the first argument on the first line? Like
> so:
> 
> 				dev_warn(i2c_dev->dev,
> 					 "timeout waiting for config load\n");
> 
> Note the extra space used for padding.
> 
[Shardar]  Will take care of this.

> > +				return -ETIMEDOUT;
> > +			}
> > +			msleep(1);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)  {
> >  	u32 val;
> >  	int err = 0;
> >  	u32 clk_divisor;
> > -	unsigned long timeout = jiffies + HZ;
> >
> >  	err = tegra_i2c_clock_enable(i2c_dev);
> >  	if (err < 0) {
> > @@ -477,20 +497,13 @@ static int tegra_i2c_init(struct tegra_i2c_dev
> *i2c_dev)
> >  	if (i2c_dev->is_multimaster_mode && i2c_dev->hw-
> >has_slcg_override_reg)
> >  		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR,
> I2C_CLKEN_OVERRIDE);
> >
> > -	if (i2c_dev->hw->has_config_load_reg) {
> > -		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD,
> I2C_CONFIG_LOAD);
> > -		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
> > -			if (time_after(jiffies, timeout)) {
> > -				dev_warn(i2c_dev->dev,
> > -					"timeout waiting for config load\n");
> > -				return -ETIMEDOUT;
> > -			}
> > -			msleep(1);
> > -		}
> > -	}
> > +	err = tegra_i2c_wait_for_config_load(i2c_dev);
> >
> >  	tegra_i2c_clock_disable(i2c_dev);
> >
> > +	if (err)
> > +		return err;
> > +
> 
> The behaviour here is now different, though I assume it's really a fix that
> disables the I2C clock in case the timeout happens. Given that I suspect this
> would be better off in a separate patch (factor out the
> tegra_i2c_wait_for_config_load() function and return error after the clocks
> have been disabled).

[Shardar] Will take care of this.
> 
> >  	if (i2c_dev->irq_disabled) {
> >  		i2c_dev->irq_disabled = 0;
> >  		enable_irq(i2c_dev->irq);
> > @@ -499,14 +512,28 @@ static int tegra_i2c_init(struct tegra_i2c_dev
> *i2c_dev)
> >  	return err;
> >  }
> >
> > +static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev
> > +*i2c_dev) {
> > +	u32 cnfg;
> > +
> > +	cnfg = i2c_readl(i2c_dev, I2C_CNFG);
> > +	if (cnfg & I2C_CNFG_PACKET_MODE_EN)
> > +		i2c_writel(i2c_dev, cnfg & (~I2C_CNFG_PACKET_MODE_EN),
> 
> Parentheses are not necessary here.
> 
> > +				I2C_CNFG);
> 
> If you drop the parentheses above this can be moved into the previous line
> because it will fit within 80 columns.

[Shardar] Will take care of this.

> 
> > +
> > +	return tegra_i2c_wait_for_config_load(i2c_dev);
> > +}
> > +
> >  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)  {
> >  	u32 status;
> >  	const u32 status_err = I2C_INT_NO_ACK |
> I2C_INT_ARBITRATION_LOST;
> >  	struct tegra_i2c_dev *i2c_dev = dev_id;
> > +	unsigned long flags;
> >
> >  	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
> >
> > +	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
> >  	if (status == 0) {
> >  		dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
> >  			 i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
> @@ -522,6 +549,7
> > @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> >  	}
> >
> >  	if (unlikely(status & status_err)) {
> > +		tegra_i2c_disable_packet_mode(i2c_dev);
> 
> If you don't care about the return value it'd be better to reflect that by
> making the function return void. I presume ignoring the error here is safe?

[Shardar] Will take care of handling the return valure.

> 
> > @@ -583,6 +614,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> *i2c_dev,
> >  	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
> >  	reinit_completion(&i2c_dev->msg_complete);
> >
> > +	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
> > +
> > +	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> > +	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> > +
> >  	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
> >  			PACKET_HEADER0_PROTOCOL_I2C |
> >  			(i2c_dev->cont_id <<
> PACKET_HEADER0_CONT_ID_SHIFT) | @@ -612,14
> > +648,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >  	if (!(msg->flags & I2C_M_RD))
> >  		tegra_i2c_fill_tx_fifo(i2c_dev);
> >
> > -	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> >  	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
> >  		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
> >  	if (msg->flags & I2C_M_RD)
> >  		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> >  	else if (i2c_dev->msg_buf_remaining)
> >  		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> > +
> >  	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> 
> Can this line be removed? The NO_ACK and ARBITRATION_LOST interrupts
> are already unmasked earlier.

[Shardar] unmask_irq() here is enabling other interrupts (PACKET_XFER_COMPLETE/TX_FIFO_DATA_REQ/RX_FIFO_DATA_REQ). please let me know if my understanding is wrong from your comment.

> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1

Re: [PATCH] i2c: tegra: proper handling of error cases

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Sat, Apr 16, 2016 at 08:08:50AM +0000, Shardar Mohammed wrote:
[...]
> > >  	if (!(msg->flags & I2C_M_RD))
> > >  		tegra_i2c_fill_tx_fifo(i2c_dev);
> > >
> > > -	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> > >  	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
> > >  		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
> > >  	if (msg->flags & I2C_M_RD)
> > >  		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> > >  	else if (i2c_dev->msg_buf_remaining)
> > >  		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> > > +
> > >  	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> > 
> > Can this line be removed? The NO_ACK and ARBITRATION_LOST interrupts
> > are already unmasked earlier.
> 
> [Shardar] unmask_irq() here is enabling other interrupts
> (PACKET_XFER_COMPLETE/TX_FIFO_DATA_REQ/RX_FIFO_DATA_REQ). please let
> me know if my understanding is wrong from your comment.

You're right, I obviously wasn't looking closely enough.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: tegra: proper handling of error cases

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

On Sat, Apr 16, 2016 at 08:08:50AM +0000, Shardar Mohammed wrote:
> Thanks for the review, updated with comments inline with Shardar.
> 
> > On Fri, Apr 15, 2016 at 06:51:47PM +0530, Shardar Shariff Md wrote:
> > > From: Shardar Shariff Md <smohammed@nvidia.com>
> > >
> > > To summarize the issue observed in error cases:
> > >
> > > In SW: For i2c message transfer, packet header and data payload is
> > > posted and then required error/packet completion interrupts are
> > > enabled later.
> > >
> > > In HW flow: HW process the packet just after packet header is posted,
> > > if ARB lost/NACK error occurs (SW will not handle immediately when
> > > error happens as error interrupts are not enabled at this point). HW
> > > assumes error is acknowledged and clears current data in FIFO, But SW
> > > here posts the remaining data payload which still stays in FIFO as
> > > stale data (data without packet header).
> > >
> > > Now once the interrupts are enabled, SW handles ARB lost/NACK error by
> > > clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended
> > > the error and will parse/process stale data (data without packet
> > > header) present in FIFO which causes invalid NACK errors.
> > >
> > > Fix: Enable the error interrupts before posting the packet into FIFO
> > > which make sure HW to not clear the fifo. Also disable the packet mode
> > > before acknowledging errors (ARB lost/NACK error) to not process any
> > > stale data.
> > >
> > > As error interrupts are enabled before posting the packet header use
> > > spinlock to avoid preempting.
> > 
> > Please try to use up the full 78 characters per line in the commit message
> > (with the exception being the subject line that should be shorter).
> 
> [Shardar] Will take care of this.

I just realized my comment was wrong. The rule is to use 72 characters
per line for the commit message. Sorry about that.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]