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 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;
};

thanks,

-- 
heikki