On Wed, Nov 05, 2014 at 02:25:59PM -0600, Thor Thayer wrote: Please leave blank lines between paragraphs and quoted bits, it makes things much easier to read than an unbroken wall of text covering many screenfuls. > On 11/05/2014 07:24 AM, Mark Brown wrote: > >To repeat again: please talk about driver problems in terms of the > >driver not in terms of a particular driver. Glancing at the code here > >it looks like spidev is buggy here, it's just blindly allowing userspace > >to overwrite the maximum speed configured for the device which seems > >like a bad idea, drivers should have no reason to expect that something > >called max_speed is actually variable. It looks like spidev is abusing > >this as a default speed. > spidev is calling the case condition SPI_IOC_WR_MAX_SPEED_HZ in > spidev_ioctl() to overwrite the maximum speed. From the code and the name, > it seems like overwriting the maximum speed was the intended use and not a > side effect. This means it was an API that was implemented thoughtlessly rather than anything else; it should be fixed to cache and use the current speed after one has been set. > >> chip->speed_hz = spi->max_speed_hz = new_spi_speed > >So the driver is overriding its idea of the current speed without > >actually updatng the hardware. Why is the fix here not to just delete > >the assignment here, it seems fairly obviously buggy? > Yes, removing line 591 of the spi-dw.c (chip->speed_hz = spi->max_speed_hz;) > will solve the problem and seems like a clean fix. In this case the > chip->speed_hz will persist the last transfer speed setting. > The chip->speed_hz parameter won't be updated as part of the spi-dw driver > initialization. This may not matter since it will get updated on the first > transfer in the (transfer->speed != speed) test shown above. OK, great - can you submit that version please?
Attachment:
signature.asc
Description: Digital signature
