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

[Maximilian Luz <luzmaximilian@gmail.com>]

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