[PATCH v10 03/12] peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

Hi Greg,

On 1/26/2019 12:29 AM, Greg Kroah-Hartman wrote:
> On Fri, Jan 25, 2019 at 10:51:54AM -0800, Jae Hyun Yoo wrote:
>> Hi Greg,
>>
>> On 1/24/2019 11:18 PM, Greg Kroah-Hartman wrote:
>>> On Thu, Jan 24, 2019 at 02:01:10PM -0800, Jae Hyun Yoo wrote:
>>>> On 1/23/2019 10:57 PM, Greg Kroah-Hartman wrote:
>>>>> On Wed, Jan 23, 2019 at 01:38:24PM -0800, Jae Hyun Yoo wrote:
>>>>>>> What about the un-initialized fields in this structure?  Can you
>>>>>>> properly handle that, and also, is this ok to be on the stack?
>>>>>>
>>>>>> It's fully initialized at here because the peci_ping_msg struct has only
>>>>>> one member:
>>>>>>
>>>>>> struct peci_ping_msg {
>>>>>> 	__u8 addr;
>>>>>> };
>>>>>
>>>>> Ok.  But my question about "can you do this off the stack" remains.
>>>>
>>>> I'll add 3 bytes of dummy padding into this structure. Also, I'll check
>>>> again u32 boundary alignment for all struct defines in peci_ioctl.h.
>>>> Would it be okay to be on stack then?
>>>
>>> The issue of being on the stack has nothing to do with alignment, and
>>> everything to do with, "can your controller handle data from the stack".
>>> Lots of busses and controllers can not (i.e. all USB devices), so you
>>> have to properly allocate all memory that is used for data transfers
>>> from areas that are able to do DMA properly (i.e. by using kmalloc).
>>>
>>> That is why I asked here about that, if this is a USB driver, having the
>>> data you wish to send from a stack variable is not allowed.  I don't
>>> know how your hardware works, which is why I was asking this.
>>>
>>> Note, some architectures (like x86), hide this fact as their stack
>>> memory is able to be DMA, so you do not run into any errors.  Other
>>> arches that Linux supports are not like that, which is why we have those
>>> types of restrictions.
>>
>> Thanks for your detailed explanation.
>>
>> In this core driver, all PECI command messages will be translated into
>> the raw PECI message structure (peci_xfer_msg) and it is the actual
>> data which will be delivered to an adapter driver through
>> adapter->xfer().
>>
>> I'll fix the translation logic to use heap instead of stack for handling
>> the peci_xfer_msg structure.
> 
> Great!  You might want to add the checks that we have in the USB core
> for this type of thing to catch users who don't remember not to send
> stack-data.  Look at usb_hcd_map_urb_for_dma() for specifics.

Thanks for the additional information. I'll add the checks as well.

Regards,
Jae