linux-kernel - Re: [RFC/PATCH] i2c/busses: Add support for Diolan U2C-12 USB/I2C adapter

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]

Message-ID: <20101104211807.07a55104@endymion.delvare>
Date:	Thu, 4 Nov 2010 21:18:07 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	guenter.roeck@...csson.com
Cc:	Ben Dooks <ben-linux@...ff.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC/PATCH] i2c/busses: Add support for Diolan U2C-12 USB/I2C 
 adapter

Hi Guenter,

On Thu, 4 Nov 2010 09:41:42 -0700, Guenter Roeck wrote:
> On Thu, 2010-11-04 at 08:43 -0400, Jean Delvare wrote:
> > I'm also unsure what is the point of having such a large buffer when
> > the largest block you ever transfer in practice is 5 bytes?
>
> I took that from the Diolan code. They always use a 257 byte temp
> buffer, since that is the maximum data size sent by the adapter. 
> You are right, I should not really need that since I don't send any long
> commands. Ultimate reason is to account for possible adapter errors, if
> it replies (or tries to reply) with more bytes than expected. Pretty
> much just playing safe.

Which commands are they using, which require such a large buffer? In
your driver, bytes are all processed one by one, which is certainly not
good performance-wise. If there is a way to read or write mode than one
byte at a time, this would be worth a try.

> > (...)
> > BTW, I'm not sure why you don't use the original buffer directly?
> > memcpy is bad performance-wise.
>
> To account for the possibility that the adapter returns more bytes than
> I am expecting. Sure, that would be a bug, but I wanted to play safe.

Hmm, OK, that makes sense.

> > (...)
> > Should be -EPROTO according to Documentation/i2c/fault-codes.
>
> Ok. Note that I got that from i2c-algo-bit.c.

I would welcome a patch fixing this.

> > > (...)
> > > +static u32 usb_func(struct i2c_adapter *a)
> > > +{
> > > +     return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > > +         I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > 
> > Odd indentation/alignment.
>
> Seems to be exactly what other drivers do, so I am a bit at loss here.

Really? Using 4 spaces for indentation is wrong. Either use a tab, or
align using 7 spaces.

> > (...)
> > You are abusing algo_data here. You are supposed to use
> > i2c_get/set_adapdata() instead. algo_data is only there for providing
> > platform specific implementation details to generic i2c algorithms such
> > as i2c-algo-bit.
>
> Copied from i2c-tiny-usb.c. I didn't really think about it. Fixed.

I take patches ;)

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/