linux-kernel - Re: [PATCH v1 11/13] serial: 8250_exar: Use BIT() in exar_ee

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 for Android: free password hash cracker in your pocket

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

Date: Fri, 3 May 2024 18:35:45 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Parker Newman <parker@...est.io>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Parker Newman <pnewman@...necttech.com>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-serial <linux-serial@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH v1 11/13] serial: 8250_exar: Use BIT() in exar_ee_read()

On Fri, May 03, 2024 at 10:26:32AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 20:20:01 +0300
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
> > On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo Järvinen wrote:
> > > On Thu, 2 May 2024, Andy Shevchenko wrote:  

..

> > > >  	// Send address to read from
> > > > -	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> > > > -		exar_ee_write_bit(priv, (ee_addr & i));
> > > > +	for (i = UART_EXAR_REGB_EE_ADDR_SIZE - 1; i >= 0; i--)
> > > > +		exar_ee_write_bit(priv, ee_addr & BIT(i));
> > > >  
> > > >  	// Read data 1 bit at a time
> > > >  	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > > > -		data <<= 1;
> > > > -		data |= exar_ee_read_bit(priv);
> > > > +		if (exar_ee_read_bit(priv))
> > > > +			data |= BIT(i);  
> > > 
> > > Does this end up reversing the order of bits? In the original, data was left
> > > shifted which moved the existing bits and added the lsb but the replacement
> > > adds highest bit on each iteration?  
> > 
> > Oh, seems a good catch!
> > 
> > I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
> > so two loops has to be aligned.
> > 
> 
> I just tested this and Ilpo is correct, the read loop portion is backwards as 
> expected. This is the corrected loop:
> 
>     // Read data 1 bit at a time
>     for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
>         if (exar_ee_read_bit(priv))
>             data |= BIT(i);
>     }
> 
> I know this looks wrong because its looping from 16->0 rather than the 
> more intuitive 15->0 for a 16bit value. This is actually correct however 
> because according to the AT93C46D datasheet there is always dummy 0 bit
> before the actual 16 bits of data.
> 
> I hope that helps,

Yes, it helps and means that we need that comment to be added to the code. Is
it the same applicable to the write part above (for address)? Because AFAIU
mine is one bit longer than yours. Maybe in the original code is a bug? Have
you tried to read high addresses?


-- 
With Best Regards,
Andy Shevchenko