Re: [PATCH v2 1/9] platform/surface: Add Surface Aggregator subsystem

[Maximilian Luz]

On 12/8/20 3:43 PM, Hans de Goede wrote:
> Hi,
>
> On 12/8/20 3:37 PM, Maximilian Luz wrote:
>
> <snip>
>
> > > > +
> > > > +    obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
> > > > +                      SSAM_SSH_DSM_REVISION, func, NULL,
> > > > +                      ACPI_TYPE_INTEGER);
> > > > +    if (!obj)
> > > > +        return -EIO;
> > > > +
> > > > +    val = obj->integer.value;
> > > > +    ACPI_FREE(obj);
> > > > +
> > > > +    if (val > U32_MAX)
> > > > +        return -ERANGE;
> > > > +
> > > > +    *ret = val;
> > > > +    return 0;
> > > > +}
> >
> > [...]
> >
> > > > +/**
> > > > + * ssam_controller_start() - Start the receiver and transmitter threads of the
> > > > + * controller.
> > > > + * @ctrl: The controller.
> > > > + *
> > > > + * Note: When this function is called, the controller should be properly
> > > > + * hooked up to the serdev core via &struct serdev_device_ops. Please refer
> > > > + * to ssam_controller_init() for more details on controller initialization.
> > > > + *
> > > > + * This function must be called from an exclusive context with regards to the
> > > > + * state, if necessary, by locking the controller via ssam_controller_lock().
> > >
> > > Again you are being a bit hand-wavy (I assume you know what I mean by that)
> > > wrt the locking requirements. If possible I would prefer clearly spelled out
> > > locking requirements in the form of "this and that lock must be held when
> > > calling this function". Preferably backed-up by lockdep_assert-s asserting
> > > these conditions.
> >
> > The reason for this is that this function specifically is currently only
> > called during initialization, when the controller has not been published
> > yet, i.e. when we have an exclusive reference to the controller.
> >
> > I'll change this to fully enforce locking (with lockdep_assert).
> >
> > > And maybe if you are a bit stricter with always holding the lock when
> > > calling this, you can also drop the WRITE_ONCE and the comment about it
> > > (in all places where you do this).
> >
> > The WRITE_ONCE is only there to ensure that the basic test in
> > ssam_request_sync_submit() can be done. I always try to be explicit
> > about access that can happen without the respective locks being held.
>
> Yes I saw the matching READ_ONCE later on (as the comment indicated
> I would), which made it more obvious to me why the WRITE_ONCE is here,'
> so maybe I should have gone back and updated this comment.

No worries, always good to have another look at these kinds of things.

> Anyways, keeping the WRITE_ONCE + READ_ONCE for this is fine.
>
> > Unfortunately it's not feasible to hold the reader lock in
> > ssam_request_sync_submit() due to reentrancy. Specifically, as the lock,
> > if at all (i.e. if this is not a client driver bound to the controller),
> > must be held not only during submission but until the request has been
> > completed. Note that if we would hold the lock during submission, this
> > is just a smoke-test.
>
> Ack.
>
> <more snip>
>
> Regards,
>
> Hans