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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, Jul 01, 2025 at 01:11:24PM +0300, Dmitry Baryshkov wrote:
> 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.

Looks better indeed.

> 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.

Pooja, I'm sorry about the extra work needed. Can you check this?

P.S. I'm already on vacation so there will be delays with my
responses.

-- 
heikki