lkml.org 
[lkml]   [2020]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 6/9] surface_aggregator: Add dedicated bus and device type
From
Date
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

\
 
 \ /
  Last update: 2020-09-24 20:16    [W:0.087 / U:1.916 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site