linux-kernel - Re: [PATCH RFC 4/4] i2c: at91: take slave mode capabilities of hardware into account

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Nov 2017 15:47:56 +0100
From:   Ludovic Desroches <ludovic.desroches@...rochip.com>
To:     Juergen Fitschen <me@....yt>
CC:     Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Wolfram Sang <wsa@...-dreams.de>, <linux-i2c@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 4/4] i2c: at91: take slave mode capabilities of
 hardware into account

On Wed, Nov 01, 2017 at 12:16:36PM +0100, Juergen Fitschen wrote:
> Hello Ludovic,
> 
> Thank you very much for your feedback!
> 
> On Tue, Oct 31, 2017 at 04:22:50PM +0100, Ludovic Desroches wrote:
> > On Fri, Oct 27, 2017 at 05:12:17PM +0200, Juergen Fitschen wrote:
> > > Some AT91 hardware has no slave mode included or only limited features
> > > (i.e. no fifos).
> > > 
> > 
> > I am wondering if it won't be better to squash this patch into the
> > previous one:
> > Without it, it seems that we can set slave_detected for the RM9200 even
> > if it doesn't support the slave mode.
> 
> Good point. I will squash both patches into one in the next version. In the
> first place I wanted to support the review process by splitting the changes in
> two patches.
> 
> > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > > index bb502c1..4a4fa67 100644
> > > --- a/drivers/i2c/busses/i2c-at91.h
> > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > @@ -107,9 +107,14 @@
> > >  
> > >  #define	AT91_TWI_VER		0x00fc	/* Version Register */
> > >  
> > > +#define	AT91_TWI_SM_AVAILABLE	BIT(0)	/* Slave mode supported */
> > > +#define	AT91_TWI_SM_CAN_NACK	BIT(1)	/* Can send NACKs in slave mode */
> > > +#define	AT91_TWI_SM_HAS_FIFO	BIT(2)	/* Has FIFO for slave mode */
> > > +
> > 
> > I would not add AT91_TWI_SM_CAN_NACK, AT91_TWI_SM_HAS_FIFO since there
> > is no code relying on them. Maybe you have some plans for the future?
> 
> Wolfram mentioned that supporting NACKs would be a welcome feature [1]. But I
> haven't implemented it, yet. The same goes for FIFO support. ATM I am not sure
> if my application will need this, since I am observing quite a lot clock
> stretching without FIFOs due to the occupied receive holding registered (RHR).
> 
> BTW: Both implementations would be kind of controversal. Without using FIFOs the
> desired NACK would be delayed by 1 byte (cf. my "artistic" ASCII graphic [2]).
> If FIFOs are enabled the delay would be even larger. So the options are:
> 
>  * No NACKs at all
>  * NACKs delayed by 1 byte, no FIFOs
>  * NACKs delayed by n byte, with FIFOs
> 
> Non of these abovementioned options is optimal and fulfill the desired behaviour
> (cf. section I2C_SLAVE_WRITE_RECEIVED of [3]).  Furthermore, AFAIK NACKs and
> FIFOs are just supported by SAMA5D2x MPUs.
> 
> These are the main reasons why I haven't implented anything related to
> AT91_TWI_SM_CAN_NACK and AT91_TWI_SM_HAS_FIFO. The designware driver ignores
> the NACK problem, as well.
> 
> Do you have an opinion on this topic?
> 

Not really, I'll discuss with the hardware guy to see how we can manage
NACKs or at least how we can improve it for next versions of the
controller.

Regards

Ludovic

> In the next version of this patchset I will remove this. I think readding these
> flags if needed shouldn't be a big deal.
> 
> 
> Best regards
>   Juergen
> 
> 
> [1] https://marc.info/?l=linux-i2c&m=150831224824540&w=2
> [2] https://marc.info/?l=linux-i2c&m=150833171430595&w=2
> [3] https://www.kernel.org/doc/Documentation/i2c/slave-interface