Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 10/07/2013 05:39 PM, Alexander Stein wrote:
> Hello Marc,
> 
> On Monday 07 October 2013 16:40:38, Marc Kleine-Budde wrote:
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index bda1888..969d3cd 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -486,6 +486,29 @@ void can_bus_off(struct net_device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(can_bus_off);
>>  
>> +static void can_berr_restart(unsigned long data)
>> +{
>> +	struct net_device *dev = (struct net_device *)data;
>> +	struct can_priv *priv = netdev_priv(dev);
>> +
>> +	netdev_dbg(dev, "berr-restart\n");
>> +	priv->do_berr_restart(dev);
>      ^^^^^^^^^^^^^^^^^^^^^^^^
> see below!
> 
>> +}
>> +
>> +void can_berr_limit(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +
>> +	if (priv->berr_limit_delay) {
>> +		netdev_dbg(dev, "berr-limit\n");
>> +		mod_timer(&priv->berr_limit_timer,
>> +			  jiffies + priv->berr_limit_delay);
>> +	} else {
>> +		priv->do_berr_restart(dev);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^

> Here you are silently requiring that do_berr_restart is non-NULL. I
> know that the driver has to properly set this when using
> can_berr_limit, but it might seem confusing.

If the driver wants to make use of this feature, it has to setup this
callback. If I add NULL pointer checks, the kernel wouldn't blow up, but
the driver doesn't work any more.

If this callback isn't setup, the userspace cannot activate the feature
via netlink.

Maybe we can define a control mode that the driver supports this feature
and during can_open() it's checked if the callback is != NULL. Than
"ifconfig up" will fail, the driver doesn't work, same as a NULL pointer
deref later, but coated in sugar :) Maybe we find a better way.

thanks for the review,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]