Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 01/07/2025 13:05, Heikki Krogerus wrote:
> On Tue, Jul 01, 2025 at 11:50:21AM +0300, Dmitry Baryshkov wrote:
>> On 01/07/2025 11:46, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
>>>> On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
>>>>> Add support for updating message out data structure for UCSI ACPI
>>>>> interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
>>>>> LPM Firmware Update.
>>>>>
>>>>> Additionally, update ucsi_send_command to accept message_out data
>>>>> and .sync_control function to pass message_out data to
>>>>> write_message_out function if the command is UCSI_SET_PDOS.
>>>>
>>>> Normally when you say "additionally" that implies that the patch should
>>>> be split up into pieces.  Why not do that here?
>>>>
>>>> And do you _really_ need to add a new parameter to all of these
>>>> functions?  It's now getting even worse, look at this:
>>>>
>>>>>    		ret = ucsi_send_command(ucsi, val,
>>>>>    					&ucsi->debugfs->response,
>>>>> -					sizeof(ucsi->debugfs->response));
>>>>> +					sizeof(ucsi->debugfs->response), NULL);
>>>>
>>>> You can kind of guess what the parameters mean before the NULL change,
>>>> but now you have to go look up "what is the last pointer for"
>>>> everywhere.
>>>>
>>>> This feels very fragile and horrible to maintain over time, please
>>>> reconsider this type of api change.
>>>
>>> So I think what Pooja was proposing in the first version of this
>>> series, where you had a dedicated function for filling the
>>> message_out, was better after all.
>>>
>>> Pooja, I'm really sorry about this, but can you revert back to that,
>>> and send it as v3? Let's start over.
>>
>> But that breaks the sync_control logic - currently it is possible to handle
>> the command in .sync_control completely. If for any reason we need to
>> implement workarounds for commands using MESSAGE_OUT field, we'd have to
>> introduce additional logic (which we just got rid of).
> 
> Okay. So how about a data structure for the entire mailbox that we can
> pass to these functions?
> 
> struct ucsi_mailbox {
>          u32 cci;
>          u64 control;
>          void *message_in;
>          size_t message_in_size;
>          void *message_out;
>          size_t message_out_size;
> };

What about a slightly different proposal (following ucsi_ccg design):


struct ucsi {
    // .....
    u32 cci;
    u8 message_in[UCSI_MAX_MESSAGE_IN];
    u8 message_out[UCSI_MAX_MESSAGE_OUT];
};

The caller will fill ucsi->message_out, call sync_control with a proper 
length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from 
ucsi->message_in.

Note: I'm positive that we can handle message buffers in this way. I'm 
not so sure about the CCI, it might be too dynamic.

> 
> thanks,
> 


-- 
With best wishes
Dmitry