Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()

[Hans de Goede <hdegoede@redhat.com>]

Hi Guenter,

Thank you for all the reviews.

On 7/15/20 6:39 PM, Guenter Roeck wrote:
> On 7/14/20 4:36 AM, Hans de Goede wrote:
>> This can be used by Type-C controller drivers which use a standard
>> usb-connector fwnode, with altmodes sub-node, to describe the available
>> altmodes.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/usb/typec.h |  7 +++++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index c9234748537a..47de2b2e3d54 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>>   }
>>   EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>>   
>> +void typec_port_register_altmodes_from_fwnode(struct typec_port *port,
>> +	const struct typec_altmode_ops *ops, void *drvdata,
>> +	struct typec_altmode **altmodes, size_t n,
>> +	struct fwnode_handle *fwnode)
>> +{
>> +	struct fwnode_handle *altmodes_node, *child;
>> +	struct typec_altmode_desc desc;
>> +	struct typec_altmode *alt;
>> +	size_t index = 0;
>> +	u32 svid, vdo;
>> +	int ret;
>> +
>> +	altmodes_node = fwnode_get_named_child_node(fwnode, "altmodes");
>> +	if (!altmodes_node)
>> +		return;
>> +
>> +	child = NULL;
>> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
>> +		ret = fwnode_property_read_u32(child, "svid", &svid);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
> 
> The properties are mandatory. I think the errors should not be ignored.

I have done this this way deliberately, let me try to explain:

We basically have 2 choices here:

1) Log an error and continue, skipping/ignoring the faulty altmode fw-child-node
2) Make the error fatal, rollback all changes made so far and return an error
to our caller

First of all, these errors should never happen and if they do happen then the
person adding the new alt-mode to the dt, will presumably test that this alt-mode
works, sees that it does not, check the logs and know why. So for stable shipping
kernels I would expect to never hit this path.

Secondly even if we somehow do hit this path in a stable shipping kernel, then
what should our caller do if we return an error? Our caller basically has 2 choices:

1. Log and otherwise ignore the error
2. Completely abort the registration of the Type-C controller, possibly breaking
things like USB over the port, charging, etc.

2. Seems rather drastic and is not necessary, except for the alt-modes all the
other functionality of the port will work fine if the call fails. So our caller
should probably choose 1. as error handling strategy. If it does that, then for
the error logging it can rely on typec_port_register_altmodes_from_fwnode() having
already logged an error, so it can just treat it as returning void.

But if our caller does not care, would it then not be better to just ignore
any bad alt-mode child nodes and still try to register an alt-mode for the
good ones?

Thirdly adding code to unwind the registration of the alt-modes done so far
+ adding code to our caller to abort the port registration would be adding a
bunch of extra, fragile code for something which should (and likely will)
never happen. So we ware adding a bunch of code here which in essence is
pretty much never tested and thus is almost assured to either be broken
from day 1, or to become broken over time.

The kernel is likely full of error handling paths with bugs because it is
trying to handle errors which never happen. Sometimes this is necessary
because e.g. a driver's probe function cannot continue without acquiring
a certain resource. But in this case we can easily avoid the broken error
handling code syndrome; and keep the code nice and simple, by just skipping
over broken nodes.

> 
>> +		}
>> +
>> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		if (index >= n) {
>> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
> 
> Seems to be pointless to continue here.

Continuing here makes sure that if the dts contains 10 alt-modes and n = 8 that we print
an error for both alt-modes which we are not registering because there is not enough space
in the passed in array for storing alt-mode pointers.

It also ensures that we error check any further nodes for missing svid/vdo properties.


> 
>> +		}
>> +
>> +		desc.svid = svid;
>> +		desc.vdo = vdo;
>> +		desc.mode = index + 1;
>> +		alt = typec_port_register_altmode(port, &desc);
>> +		if (IS_ERR(alt)) {
>> +			dev_err(&port->dev, "Error registering altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
> 
> Maybe there is a reason to ignore all those errors. If so,
> that should be explained.

See above, note this is another error which should never happen, I think
this can only happen in case of -ENOMEM, which itself can only happen
for allocations of an order greater then n=2 AFAIK, and I don't think
typec_port_register_altmode() does any such allocations.

I guess some comment here is warranted, but my full explanation above
is way too long. So maybe a simple comment like this?  :

			/* Should never happen, keep the error handling as simple as possible */

Regards,

Hans


> 
>> +		}
>> +
>> +		alt->ops = ops;
>> +		typec_altmode_set_drvdata(alt, drvdata);
>> +		altmodes[index] = alt;
>> +		index++;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
>> +
>>   /**
>>    * typec_register_port - Register a USB Type-C Port
>>    * @parent: Parent device
>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
>> index 5daa1c49761c..fbe4bccb3a98 100644
>> --- a/include/linux/usb/typec.h
>> +++ b/include/linux/usb/typec.h
>> @@ -17,6 +17,7 @@ struct typec_partner;
>>   struct typec_cable;
>>   struct typec_plug;
>>   struct typec_port;
>> +struct typec_altmode_ops;
>>   
>>   struct fwnode_handle;
>>   struct device;
>> @@ -121,6 +122,12 @@ struct typec_altmode
>>   struct typec_altmode
>>   *typec_port_register_altmode(struct typec_port *port,
>>   			     const struct typec_altmode_desc *desc);
>> +
>> +void typec_port_register_altmodes_from_fwnode(struct typec_port *port,
>> +	const struct typec_altmode_ops *ops, void *drvdata,
>> +	struct typec_altmode **altmodes, size_t n,
>> +	struct fwnode_handle *fwnode);
>> +
>>   void typec_unregister_altmode(struct typec_altmode *altmode);
>>   
>>   struct typec_port *typec_altmode2port(struct typec_altmode *alt);
>>
>