lkml.org 
[lkml]   [2014]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
From
On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:

<snip>

>> + /*
>> + * Buffer to hold the packet for read or write transfers. One
>> + * is enough since we can't have multiple transfers in
>> + * parallel on the i2c adapter.
>> + */
>> + union {
>> + struct {
>> + u8 port;
>> + u8 addr;
>> + u8 mem_addr_len;
>> + __le32 mem_addr;
>> + __le16 buf_len;
>> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> + } __packed tx;
>> + struct {
>> + __le16 buf_len;
>> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> + } __packed rx;
>> + } buf;
>
> While this works in this case due to the extra copy you do in
> dln2_transfer, allocating buffers that would (generally) be used for DMA
> transfers as part of a larger structure is a recipe for trouble.
>
> It's probably better to allocate separately, if only to prevent people
> from thinking there might be a bug here.
>

Just to make sure I understand this, what could the issues be? The
buffers not being aligned or not allocated in continuous physical
memory?

<snip>

>> +
>> + rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
>> + if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
>> + return -EPROTO;
>> +
>> + if (data_len > rx_buf_len)
>> + data_len = rx_buf_len;
>
> You're still not checking that the received data does not overflow the
> supplied buffer as I already commented on v3.
>
>> +
>> + memcpy(data, dln2->buf.rx.buf, data_len);
>> +
>> + return data_len;
>> +}

Hmm, perhaps I am missing something, but we never transfer more then
data_len, where data_len is the size of the buffer supplied by the
user.

<snip>

>> +
>> + platform_set_drvdata(pdev, dln2);
>> +
>> + ret = device_create_file(dev, &dev_attr_freq);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to add freq attribute\n");
>> + return ret;
>> + }
>
> There are a couple of problems here. First, you should not make this an
> attribute of the platform device, which is created before any driver is
> bound (might not ever happen).
>
> Instead add the attribute to the i2c adapter below. However, you need to
> do this using device attribute groups to avoid racing with userspace (as
> you are when using device_create_file after the device itself has been
> created).
>
> You should probably also make your attribute name less generic by adding
> a "dln2_"-prefix.

Thanks for the detailed review and explanations, as always :)


\
 
 \ /
  Last update: 2014-09-17 12:41    [W:0.130 / U:0.528 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site