[PATCH] drm/udl: Refactor edid retreiving in UDL driver

[Hans de Goede]

Hi,

On 14-03-19 11:00, Jani Nikula wrote:
> On Wed, 13 Mar 2019, Robert Tarasov <tutankhamen at chromium.org> wrote:
>> On Wed, Mar 13, 2019 at 2:26 AM Jani Nikula <jani.nikula at linux.intel.com>
>> wrote:
>>   read_buff = kmalloc(2, GFP_KERNEL);
>>>
>>> A follow-up cleanup might be to switch to using "u8 read_buff[2];"
>>> instead of kmallocing it.
>>>
>>> I don't claim to understand how the usb stuff works, but otherwise the
>>> patch looks good to me. Nice refactoring!
>>>
>>>
>> "The buffer passed to usb_control_msg may end up in scatter-gather list,
>> and may thus not be on the stack. Having it on the stack usually works on
>> x86,   but not on other archs. "
> 
> Apparently you're referring to commit 242187b36255 ("udldrmfb:
> udl_get_edid: usb_control_msg buffer must not be on the stack"). Cc:
> Hans.

Right, that is correct, note this is enforced by the usb-core, having
the buffer on the stack will trigger a WARN_ONCE in drivers/usb/core/hcd.c:
usb_hcd_map_urb_for_dma() even on x86.

> Mind blows. Not a word on this anywhere in usb_control_msg()
> documentation. Nothing. I found this patch [1] that never go applied,
> which doesn't actually say anything about stack, only alignment and
> padding. Cc: Mauro.

I guess this is something everyone who works regularly on USB drivers
just learned once the hard way by triggering (and then fixing) the
WARN_ONCE. Note I'm not trying to make an excuse for the lacking docs
here, just trying to provide some background. I fully agree that the
docs should be updated to say that the buffer must be dma-able and
thus must not be on the stack. Maybe the docs should simply say
something along the lines of:

"the buffer should be dma-capable and properly aligned for dma,
it is advisable to use a dedictated kmalloc-ed buffer for this"

Regards,

Hans