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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> > 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



--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux