[3/5] i2c: aspeed: fix master pending state handling
diff mbox series

Message ID 20191007231313.4700-4-jae.hyun.yoo@linux.intel.com
State Changes Requested
Headers show
Series
  • i2c: aspeed: Add buffer and DMA modes support
Related show

Commit Message

Jae Hyun Yoo Oct. 7, 2019, 11:13 p.m. UTC
In case of master pending state, it should not trigger the master
command because this H/W is sharing the same data buffer for slave
and master operations, so this commit fixes the issue with making
the master command triggering happen when the state goes to active
state.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Brendan Higgins Oct. 8, 2019, 8:31 p.m. UTC | #1
On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger the master
> command because this H/W is sharing the same data buffer for slave
> and master operations, so this commit fixes the issue with making
> the master command triggering happen when the state goes to active
> state.

nit: Makes sense, but can you explain what might happen without your
change?

> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Thanks!
Jae Hyun Yoo Oct. 8, 2019, 9:13 p.m. UTC | #2
On 10/8/2019 1:31 PM, Brendan Higgins wrote:
> On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
>> In case of master pending state, it should not trigger the master
>> command because this H/W is sharing the same data buffer for slave
>> and master operations, so this commit fixes the issue with making
>> the master command triggering happen when the state goes to active
>> state.
> 
> nit: Makes sense, but can you explain what might happen without your
> change?

If we don't use this fix, a master command could corrupt data in the
shared buffer if H/W is still on processing slave operation at the
timing.

>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> 
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Thanks a lot for your review!

-Jae
Brendan Higgins Oct. 8, 2019, 9:54 p.m. UTC | #3
On Tue, Oct 8, 2019 at 2:13 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> On 10/8/2019 1:31 PM, Brendan Higgins wrote:
> > On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
> >> In case of master pending state, it should not trigger the master
> >> command because this H/W is sharing the same data buffer for slave
> >> and master operations, so this commit fixes the issue with making
> >> the master command triggering happen when the state goes to active
> >> state.
> >
> > nit: Makes sense, but can you explain what might happen without your
> > change?
>
> If we don't use this fix, a master command could corrupt data in the
> shared buffer if H/W is still on processing slave operation at the
> timing.

Right, can you add that to the commit message?

Is this trivially reproducible? We might want to submit this
separately as a bugfix.

Actually yeah, can you send this separately as a bugfix? I think we
might want to include this in 5.4.

Wolfram and Joel, what do you think?
Tao Ren Oct. 8, 2019, 10 p.m. UTC | #4
On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger the master
> command because this H/W is sharing the same data buffer for slave
> and master operations, so this commit fixes the issue with making
> the master command triggering happen when the state goes to active
> state.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index fa66951b05d0..40f6cf98d32e 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>  	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>  	u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>  
> -	bus->master_state = ASPEED_I2C_MASTER_START;
> -
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	/*
>  	 * If it's requested in the middle of a slave session, set the master
>  	 * state to 'pending' then H/W will continue handling this master
>  	 * command when the bus comes back to the idle state.
>  	 */
> -	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
> +	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>  		bus->master_state = ASPEED_I2C_MASTER_PENDING;
> +		return;
> +	}
>  #endif /* CONFIG_I2C_SLAVE */
>  
> +	bus->master_state = ASPEED_I2C_MASTER_START;
>  	bus->buf_index = 0;
>  
>  	if (msg->flags & I2C_M_RD) {
> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  		if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>  			goto out_no_complete;
>  
> -		bus->master_state = ASPEED_I2C_MASTER_START;
> +		aspeed_i2c_do_start(bus);
>  	}

Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
master transaction cannot be restarted when aspeed-i2c is running in slave state and
receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.


Cheers,

Tao
Jae Hyun Yoo Oct. 8, 2019, 10:45 p.m. UTC | #5
Hi Tao,

On 10/8/2019 3:00 PM, Tao Ren wrote:
> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
>> In case of master pending state, it should not trigger the master
>> command because this H/W is sharing the same data buffer for slave
>> and master operations, so this commit fixes the issue with making
>> the master command triggering happen when the state goes to active
>> state.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>>   drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index fa66951b05d0..40f6cf98d32e 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>>   	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>>   	u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>>   
>> -	bus->master_state = ASPEED_I2C_MASTER_START;
>> -
>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>   	/*
>>   	 * If it's requested in the middle of a slave session, set the master
>>   	 * state to 'pending' then H/W will continue handling this master
>>   	 * command when the bus comes back to the idle state.
>>   	 */
>> -	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>> +	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>>   		bus->master_state = ASPEED_I2C_MASTER_PENDING;
>> +		return;
>> +	}
>>   #endif /* CONFIG_I2C_SLAVE */
>>   
>> +	bus->master_state = ASPEED_I2C_MASTER_START;
>>   	bus->buf_index = 0;
>>   
>>   	if (msg->flags & I2C_M_RD) {
>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   		if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>   			goto out_no_complete;
>>   
>> -		bus->master_state = ASPEED_I2C_MASTER_START;
>> +		aspeed_i2c_do_start(bus);
>>   	}
> 
> Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
> master transaction cannot be restarted when aspeed-i2c is running in slave state and
> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.

Even in that case, master can be restarted properly because slave_irq
will be called first because master is in MASTER_PENDING state, so the
slave_irq handles the STOP interrupt as well, and then master_irq will
be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be
called eventually.

Also, this is right point to call the aspeed_i2c_do_start
because master state will be changed to MASTER_START by the
aspeed_i2c_do_start and we have to do remaining handling for the
MASTER_START in the master_irq by falling through after the call.

Thanks,

Jae
Jae Hyun Yoo Oct. 8, 2019, 10:55 p.m. UTC | #6
On 10/8/2019 2:54 PM, Brendan Higgins wrote:
> On Tue, Oct 8, 2019 at 2:13 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> On 10/8/2019 1:31 PM, Brendan Higgins wrote:
>>> On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
>>>> In case of master pending state, it should not trigger the master
>>>> command because this H/W is sharing the same data buffer for slave
>>>> and master operations, so this commit fixes the issue with making
>>>> the master command triggering happen when the state goes to active
>>>> state.
>>>
>>> nit: Makes sense, but can you explain what might happen without your
>>> change?
>>
>> If we don't use this fix, a master command could corrupt data in the
>> shared buffer if H/W is still on processing slave operation at the
>> timing.
> 
> Right, can you add that to the commit message?

Sure, will do that.

> Is this trivially reproducible? We might want to submit this
> separately as a bugfix.

It's very rarely observed.

> Actually yeah, can you send this separately as a bugfix? I think we
> might want to include this in 5.4.

Why not. I can send it separately but this patch series should wait for
merging the bug fix to avoid context conflicts.

> Wolfram and Joel, what do you think?
>
Tao Ren Oct. 8, 2019, 11:15 p.m. UTC | #7
On 10/8/19 3:45 PM, Jae Hyun Yoo wrote:
> Hi Tao,
> 
> On 10/8/2019 3:00 PM, Tao Ren wrote:
>> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
>>> In case of master pending state, it should not trigger the master
>>> command because this H/W is sharing the same data buffer for slave
>>> and master operations, so this commit fixes the issue with making
>>> the master command triggering happen when the state goes to active
>>> state.
>>>
>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>> ---
>>>   drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>> index fa66951b05d0..40f6cf98d32e 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>>>       struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>>>       u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>>>   -    bus->master_state = ASPEED_I2C_MASTER_START;
>>> -
>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>       /*
>>>        * If it's requested in the middle of a slave session, set the master
>>>        * state to 'pending' then H/W will continue handling this master
>>>        * command when the bus comes back to the idle state.
>>>        */
>>> -    if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>> +    if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>>>           bus->master_state = ASPEED_I2C_MASTER_PENDING;
>>> +        return;
>>> +    }
>>>   #endif /* CONFIG_I2C_SLAVE */
>>>   +    bus->master_state = ASPEED_I2C_MASTER_START;
>>>       bus->buf_index = 0;
>>>         if (msg->flags & I2C_M_RD) {
>>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>>           if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>>               goto out_no_complete;
>>>   -        bus->master_state = ASPEED_I2C_MASTER_START;
>>> +        aspeed_i2c_do_start(bus);
>>>       }
>>
>> Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
>> master transaction cannot be restarted when aspeed-i2c is running in slave state and
>> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.
> 
> Even in that case, master can be restarted properly because slave_irq
> will be called first because master is in MASTER_PENDING state, so the
> slave_irq handles the STOP interrupt as well, and then master_irq will
> be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be
> called eventually.

I mean master_irq cannot be called when irq_remaining becomes 0 after slave_irq.


Cheers,

Tao
Jae Hyun Yoo Oct. 8, 2019, 11:28 p.m. UTC | #8
On 10/8/2019 4:15 PM, Tao Ren wrote:
> On 10/8/19 3:45 PM, Jae Hyun Yoo wrote:
>> Hi Tao,
>>
>> On 10/8/2019 3:00 PM, Tao Ren wrote:
>>> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
>>>> In case of master pending state, it should not trigger the master
>>>> command because this H/W is sharing the same data buffer for slave
>>>> and master operations, so this commit fixes the issue with making
>>>> the master command triggering happen when the state goes to active
>>>> state.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>> ---
>>>>    drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>> index fa66951b05d0..40f6cf98d32e 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>>>>        struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>>>>        u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>>>>    -    bus->master_state = ASPEED_I2C_MASTER_START;
>>>> -
>>>>    #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>        /*
>>>>         * If it's requested in the middle of a slave session, set the master
>>>>         * state to 'pending' then H/W will continue handling this master
>>>>         * command when the bus comes back to the idle state.
>>>>         */
>>>> -    if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>>> +    if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>>>>            bus->master_state = ASPEED_I2C_MASTER_PENDING;
>>>> +        return;
>>>> +    }
>>>>    #endif /* CONFIG_I2C_SLAVE */
>>>>    +    bus->master_state = ASPEED_I2C_MASTER_START;
>>>>        bus->buf_index = 0;
>>>>          if (msg->flags & I2C_M_RD) {
>>>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>>>            if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>>>                goto out_no_complete;
>>>>    -        bus->master_state = ASPEED_I2C_MASTER_START;
>>>> +        aspeed_i2c_do_start(bus);
>>>>        }
>>>
>>> Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
>>> master transaction cannot be restarted when aspeed-i2c is running in slave state and
>>> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.
>>
>> Even in that case, master can be restarted properly because slave_irq
>> will be called first because master is in MASTER_PENDING state, so the
>> slave_irq handles the STOP interrupt as well, and then master_irq will
>> be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be
>> called eventually.
> 
> I mean master_irq cannot be called when irq_remaining becomes 0 after slave_irq.

Ah, I see. It would be possibly happened. Probably we need to remove
'if (irq_remaining)' checking in bus_irq to make it call irqs always.
Will fix the issue in the next round.

Thanks,

Jae
Joel Stanley Oct. 10, 2019, 5:28 a.m. UTC | #9
On Tue, 8 Oct 2019 at 21:54, Brendan Higgins <brendanhiggins@google.com> wrote:
>
> On Tue, Oct 8, 2019 at 2:13 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
> >
> > On 10/8/2019 1:31 PM, Brendan Higgins wrote:
> > > On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
> > >> In case of master pending state, it should not trigger the master
> > >> command because this H/W is sharing the same data buffer for slave
> > >> and master operations, so this commit fixes the issue with making
> > >> the master command triggering happen when the state goes to active
> > >> state.
> > >
> > > nit: Makes sense, but can you explain what might happen without your
> > > change?
> >
> > If we don't use this fix, a master command could corrupt data in the
> > shared buffer if H/W is still on processing slave operation at the
> > timing.
>
> Right, can you add that to the commit message?
>
> Is this trivially reproducible? We might want to submit this
> separately as a bugfix.
>
> Actually yeah, can you send this separately as a bugfix? I think we
> might want to include this in 5.4.
>
> Wolfram and Joel, what do you think?

Yes, good suggestion. A corruption fix should be merged I think.

Always send bug fixes upstream with Fixes tags so they land in the
stable tree. This is preferable to sending them separately to the
openbmc for inclusion in that tree, and potentially reaches a wider
audience.

Cheers,

Joel

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index fa66951b05d0..40f6cf98d32e 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -336,18 +336,19 @@  static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
 	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
 	u8 slave_addr = i2c_8bit_addr_from_msg(msg);
 
-	bus->master_state = ASPEED_I2C_MASTER_START;
-
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	/*
 	 * If it's requested in the middle of a slave session, set the master
 	 * state to 'pending' then H/W will continue handling this master
 	 * command when the bus comes back to the idle state.
 	 */
-	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
+	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
 		bus->master_state = ASPEED_I2C_MASTER_PENDING;
+		return;
+	}
 #endif /* CONFIG_I2C_SLAVE */
 
+	bus->master_state = ASPEED_I2C_MASTER_START;
 	bus->buf_index = 0;
 
 	if (msg->flags & I2C_M_RD) {
@@ -432,7 +433,7 @@  static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
 			goto out_no_complete;
 
-		bus->master_state = ASPEED_I2C_MASTER_START;
+		aspeed_i2c_do_start(bus);
 	}
 #endif /* CONFIG_I2C_SLAVE */