Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun <yorksun@freescale.com>]

> > That's the point. Your patch doesn't check if the length is valid. You
> > rely on the caller to know no more than 32 bytes can be transferred. It
> > shouldn't limit the subfunction to transfer more than 32 bytes if
> > hardware can support it by multiple transactions. If not, print out an
> > error message.
> > 
> 
> It is customary for kernel functions to only validate parameters
> wherever a value enters or leaves the kernel, or at least a logical
> boundary. Anything else would blow up the kernel size to a point where
> it would be unusable. The patch checks if the block length received from
> the SMBus slave is correct, which is exactly what it is supposed to do.
> 
> If you look closely, you may realize that the mpc_read also doesn't
> validate of any of the other parameters. Are you going to request that
> such validations be added as well ? How about the other functions in
> this driver ? Should each function also validate each of its
> parameters ? If not, where do you draw the line ?
> 
> Besides, the caller is the SMBus block read function, which does know
> that SMBus block reads are limited to 32 bytes (plus length).
> 

I am going to let it go for now. Obviously your patch is working when
length=1. Probably it will never be called with length other than 1 for
SMBus block read. It would be nicer if you could put a comment there
just in case in the future someone runs into a case where length+=byte
causes a problem.

York