Message-ID: <bdb02c89-2290-f53f-65a1-fd2b42fc98a2@gmail.com>
Date:   Wed, 23 Sep 2020 23:12:49 +0200
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
        Blaž Hrastnik <blaz@...n.io>,
        Dorian Stoll <dorian.stoll@...p.io>
Subject: Re: [RFC PATCH 6/9] surface_aggregator: Add dedicated bus and device
 type

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.

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

>> +/**
>> + * 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.

>> +/**
>> + * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
>> + * parameters.
>> + * @d:   Domain of the device.
>> + * @cat: Target category of the device.
>> + * @tid: Target ID of the device.
>> + * @iid: Instance ID of the device.
>> + * @fun: Sub-function of the device.
>> + *
>> + * Initializes a &struct ssam_device_id with the given parameters. See &struct
>> + * ssam_device_uid for details regarding the parameters. The special values
>> + * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
>> + * matching should ignore target ID, instance ID, and/or sub-function,
>> + * respectively. This macro initializes the ``match_flags`` field based on the
>> + * given parameters.
>> + */
>> +#define SSAM_DEVICE(d, cat, tid, iid, fun)					\
>> +	.match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)	\
>> +		     | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)	\
>> +		     | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),	\
>> +	.domain   = d,								\
>> +	.category = cat,							\
>> +	.target   = ((tid) != SSAM_ANY_TID) ? (tid) : 0,			\
>> +	.instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,			\
>> +	.function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0				\
> 
> Again, the 16 vs 8 bits here feels odd.  No casting???

I could add casts, but I don't really see any reason why. All valid
inputs for this macro are either u8 or SSAM_ANY_[TID,IID,FUN], for the
respective parameter. Other values are considered invalid. So the
assignment should, if used correctly, only ever assign u8 (zero in case
of the one special value). I do somewhat hope that the compiler would
complain about an overflow if someone entered any non-special fixed
value larger than U8_MAX if I don't cast here. Casting would remove
those warnings. I should probably explicitly say that the input type is
u8 in the documentation though.

>> +/**
>> + * ssam_device_get() - Increment reference count of SSAM client device.
>> + * @sdev: The device to increment the reference count of.
>> + *
>> + * Increments the reference count of the given SSAM client device by
>> + * incrementing the reference count of the enclosed &struct device via
>> + * get_device().
>> + *
>> + * See ssam_device_put() for the counter-part of this function.
>> + *
>> + * Return: Returns the device provided as input.
>> + */
>> +static inline struct ssam_device *ssam_device_get(struct ssam_device *sdev)
>> +{
>> +	get_device(&sdev->dev);
>> +	return sdev;
> 
> Do you want to check if sdev is NULL or not here before referencing
> it?

That would be a good idea.

>> +}
>> +
>> +/**
>> + * ssam_device_put() - Decrement reference count of SSAM client device.
>> + * @sdev: The device to decrement the reference count of.
>> + *
>> + * Decrements the reference count of the given SSAM client device by
>> + * decrementing the reference count of the enclosed &struct device via
>> + * put_device().
>> + *
>> + * See ssam_device_get() for the counter-part of this function.
>> + */
>> +static inline void ssam_device_put(struct ssam_device *sdev)
>> +{
>> +	put_device(&sdev->dev);
> 
> Same here, do you need to check?
> 
> anyway, again, nice work, if only all of my code reviews were this easy
> :)

Wow, thank you for the praise!

Regards,
Max

Powered by blists - more mailing lists