Re: [RFC PATCH 6/9] surface_aggregator: Add dedicated bus and device type

[Maximilian Luz]

On 9/24/20 9:12 AM, Greg Kroah-Hartman wrote:
> On Wed, Sep 23, 2020 at 11:12:49PM +0200, Maximilian Luz wrote:
> > On 9/23/20 7:33 PM, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 23, 2020 at 05:15:08PM +0200, Maximilian Luz wrote:
> > [...]
> >
> > > Overall, nice work on this patch, the integration to the driver core
> > > looks totally correct.  Great job.
> >
> > Thanks!
> >
> > > A few minor nits below:
> > >
> > > > --- /dev/null
> > > > +++ b/drivers/misc/surface_aggregator/bus.c
> > > > @@ -0,0 +1,419 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +
> > >
> > > No copyright?
> >
> > As with the other files, I forgot to add that.
> >
> > [...]
> >
> > > > +int ssam_device_add(struct ssam_device *sdev)
> > > > +{
> > > > +	int status;
> > > > +
> > > > +	/*
> > > > +	 * Ensure that we can only add new devices to a controller if it has
> > > > +	 * been started and is not going away soon. This works in combination
> > > > +	 * with ssam_controller_remove_clients to ensure driver presence for the
> > > > +	 * controller device, i.e. it ensures that the controller (sdev->ctrl)
> > > > +	 * is always valid and can be used for requests as long as the client
> > > > +	 * device we add here is registered as child under it. This essentially
> > > > +	 * guarantees that the client driver can always expect the preconditions
> > > > +	 * for functions like ssam_request_sync (controller has to be started
> > > > +	 * and is not suspended) to hold and thus does not have to check for
> > > > +	 * them.
> > > > +	 *
> > > > +	 * Note that for this to work, the controller has to be a parent device.
> > > > +	 * If it is not a direct parent, care has to be taken that the device is
> > > > +	 * removed via ssam_device_remove(), as device_unregister does not
> > > > +	 * remove child devices recursively.
> > > > +	 */
> > > > +	ssam_controller_statelock(sdev->ctrl);
> > > > +
> > > > +	if (READ_ONCE(sdev->ctrl->state) != SSAM_CONTROLLER_STARTED) {
> > >
> > > You locked the state, why the READ_ONCE()?  Is taht needed?
> >
> > At this point, no. I have, at some point, decided that, since I do
> > access the state outside of that lock at some point (specifically when
> > submitting the request in ssam_request_sync_submit() to detect mis-use
> > of the AP), that I'm going to mark them all as READ_ONCE. Mostly
> > because, due to that one check, I have to set the state via WRITE_ONCE.
> > Note that that check accessing it outside of the lock is a very basic
> > validity check and actually doesn't guarantee _anything_. Again, it's
> > just there to try and spot bad API usage. Every actually valid access to
> > the state should be locked, so the rest doesn't need the READ_ONCE. I
> > can remove those if you want me to.
>
> I would remove the ones you don't really need, but as you are doing this
> also to show intent, that should be fine.

Alright, I'll do that.

> > > > +		ssam_controller_stateunlock(sdev->ctrl);
> > > > +		return -ENXIO;
> > >
> > > odd error value, why this one?
> >
> > I generally use -ENXIO to indicate that the controller device is not
> > present, has not been initialized yet, or is being/has been shut down.
> > The error here will be caused by the controller going away (or having
> > been suspended) after the device has been created and befor the device
> > is added. I guess in case of shutdown, -ESHUTDOWN may be better, but
> > then I'm not sure what to return when the controller is suspended.
>
> Do you really need different error values?

No, not really. -ESHUTDOWN just kind of feels wrong to me for a
suspended device (specifically as that's already returned when packets
are force-evicted when the controller is shutting down).

> Anyway, it's fine, that just seemed like an odd error for that case, but
> any error is ok.

Okay, I guess I'll keep it for now. If you or anyone else have any ideas
for replacements, I'm open to them.

> > > > +/**
> > > > + * struct ssam_device_uid - Unique identifier for SSAM device.
> > > > + * @domain:   Domain of the device.
> > > > + * @category: Target category of the device.
> > > > + * @target:   Target ID of the device.
> > > > + * @instance: Instance ID of the device.
> > > > + * @function: Sub-function of the device. This field can be used to split a
> > > > + *            single SAM device into multiple virtual subdevices to separate
> > > > + *            different functionality of that device and allow one driver per
> > > > + *            such functionality.
> > > > + */
> > > > +struct ssam_device_uid {
> > > > +	u8 domain;
> > > > +	u8 category;
> > > > +	u8 target;
> > > > +	u8 instance;
> > > > +	u8 function;
> > > > +};
> > > > +
> > > > +/*
> > > > + * Special values for device matching.
> > > > + */
> > > > +#define SSAM_ANY_TID		0xffff
> > > > +#define SSAM_ANY_IID		0xffff
> > > > +#define SSAM_ANY_FUN		0xffff
> > >
> > > These are 16 bits, but the uid values above are 8 bits.  How does that
> > > match up?
> >
> > Those values are only intended for use with the SSAM_DEVICE() macro,
> > where they are used to set the match flags. They're u16 so that they
> > don't interfere with any potentially valid ID value (0x00 to 0xff). The
> > lowest byte is specifically 0xff to make it easier to spot potential
> > mis-use in the struct above, as that's an ID that, as far as I know,
> > doesn't have any valid use (at least yet). They should never be used
> > directly with the struct above, something I should probably clarify in
> > the documentation.
>
> Yes, documenting it would make more sense, the 8 vs. 16 threw me off
> here.

Will do that.

Thank you,
Max