linux-kernel - Re: [PATCH 02/16] bus: mhi: core: Add support for registering MHI controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e32b0a53-ce95-6d73-46c6-76d17af37146@codeaurora.org>
Date:   Fri, 24 Jan 2020 11:12:57 -0700
From:   Jeffrey Hugo <jhugo@...eaurora.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        arnd@...db.de, smohanad@...eaurora.org, kvalo@...eaurora.org,
        bjorn.andersson@...aro.org, hemantk@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/16] bus: mhi: core: Add support for registering MHI
 controllers

On 1/24/2020 10:47 AM, Greg KH wrote:
> On Fri, Jan 24, 2020 at 07:24:43AM -0700, Jeffrey Hugo wrote:
>>>> +/**
>>>> + * struct mhi_result - Completed buffer information
>>>> + * @buf_addr: Address of data buffer
>>>> + * @dir: Channel direction
>>>> + * @bytes_xfer: # of bytes transferred
>>>> + * @transaction_status: Status of last transaction
>>>> + */
>>>> +struct mhi_result {
>>>> +	void *buf_addr;
>>>
>>> Why void *?
>>
>> Because its not possible to resolve this more clearly.  The client provides
>> the buffer and knows what the structure is.  The bus does not. Its just an
>> opaque pointer (hence void *) to the bus, and the client needs to decode it.
>> This is the struct that is handed to the client to allow them to decode the
>> activity (either a received buf, or a confirmation that a transmitted buf
>> has been consumed).
> 
> Then shouldn't this be a "u8 *" instead as you are saying how many bytes
> are here?

I'm sorry, I don't see the benefit of that.  Can you elaborate on why 
you think that u8 * is a better type?

Sure, its an arbitrary byte stream from the perspective of the bus, but 
to the client, 99% of the time its going to have some structure.

In the call back, the first thing the client is likely to do is:
struct my_struct *s = buf_addr;

This works great when its a void *.  If buf_addr is a u8 *, that's not 
valid, and will result in a compiler error (at-least per gcc 5.4.0).
With u8 *, the client has to do:
struct my_struct *s = (struct my_struct *)buf_addr;

I don't see a benefit to u8 * over void * in this case.

The only possibly benefit I might see is if the client wants to use 
buf_addr as an array to poke into it and maybe check a magic number, but 
that assumes said magic number is a u8.  Otherwise the client has to do 
an explicit cast.  It seems like such a small amount of the time that 
usecase would be valid, that its not worth it to cater to it.

rpmsg, as one example, does the exact same thing where the received 
buffer is a void *, and there is a size parameter.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.