Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Wed, Jul 16, 2025 at 01:39:53PM +0200, Radu Vele wrote:
> On Tue, Jul 15, 2025 at 8:07 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > On Mon, Jul 14, 2025 at 10:32:03AM +0200, Radu Vele wrote:
> > > On Fri, Jul 11, 2025 at 6:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > >
> > > > On Fri, Jul 11, 2025 at 12:35:02AM +0000, Radu Vele wrote:
> > > > > Add a lock associated to each port to protect port data against
> > > > > concurrent access. Concurrency may result from sysfs commands
> > > > > and ec events.
> > > >
> > > > I realized the critical sections are way too large.  What exactly data the
> > > > lock tries to protect?  Is the race possibility introduced by any previous
> > > > commits?  Please provide more context.
> > >
> > > With the implementation of the role swap operations from the previous
> > > commit (and also enter usb mode from another recent commit) we
> > > introduce the possibility of concurrent access to the cros_ec_typec port
> > > data from the userspace (e.g. trigger a power role swap from sysfs) vs
> > > from EC events (e.g. partner triggered a role swap that we accept).
> > > This is the main reason to propose a per-port lock. This way we ensure
> > > we protect the state of each port in the cros_ec_typec driver.
> >
> > To make sure I understand, did you mean the lock tries to prevent from
> > sending multiple commands to EC at a time?  If yes, does it still need
> > if the underlying ec_dev is guranteed that [1]?
> 
> Not really, as I noticed that both the ec and Type-C connector class
> use their own mutexes.
> 
> My intention with the mutexes is to avoid race conditions in the case
> when a role swap is in progress but at the same time there is a
> `cros_port_update` that modifies the state of the port. Another
> example I have in mind is when the port is being unregistered and
> a role swap is issued.

The critical section is too large to understand which fields in the data
structure it tries to protect.  Please review again whether the lock is
needed or not and shrink the critical section if possible.

If taking the changes in cros_typec_perform_role_swap() as another example:

    static int cros_typec_perform_role_swap(...)
    {
        ...
        struct ec_params_usb_pd_control req;

        guard(mutex)(&port->lock);

        req...
        cros_ec_cmd(...)

        req...
        cros_ec_cmd(...)

        switch (...) {

            typec_set_data_role...

            typec_set_pwr_role...
        }
    }

The `req` and `cros_ec_cmd` obviously don't need to protect.  Does
typec_set_data_role() and typec_set_pwr_role() need to protect from
concurrent calling?