[RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Marc Kleine-Budde]

Until commit

    08da7da41ea4 can: provide a separate bittiming_const parameter to
                 bittiming functions

it was possible to have devices not providing bittiming_const. This can
be used for hardware that only support pre-defined fixed bitrates.
Although no mainline driver is using this feature so far.

This patch re-introduces this feature for the bitrate and the data
bitrate (of CANFD controllers). The driver can specify the
{data_,}bittiming_const (if the bittiming parameters should be
calculated from the bittiming_const) as before or no
{data_,}bittiming_const but implement the do_set_{data,}bittiming
callback.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hello,

this patch should make the mcba_usb driver, which only support fixed bitrates,
a bit simpler.

regards,
Marc

 drivers/net/can/dev.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8d6208c0b400..c240cd9f823b 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -284,10 +284,6 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
 {
 	int err;
 
-	/* Check if the CAN device has bit-timing parameters */
-	if (!btc)
-		return -EOPNOTSUPP;
-
 	/*
 	 * Depending on the given can_bittiming parameter structure the CAN
 	 * timing parameters are calculated based on the provided bitrate OR
@@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
 		/* Do not allow changing bittiming while running */
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
+
+		/* Calculate bittiming parameters based on
+		 * bittiming_const if set, otherwise pass bitrate
+		 * directly via do_set_bitrate(). Bail out if neither
+		 * is given.
+		 */
+		if (!priv->bittiming_const && !priv->do_set_bittiming)
+			return -EOPNOTSUPP;
+
 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
-		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
-		if (err)
-			return err;
+		if (priv->bittiming_const) {
+			err = can_get_bittiming(dev, &bt,
+						priv->bittiming_const);
+			if (err)
+				return err;
+		}
 		memcpy(&priv->bittiming, &bt, sizeof(bt));
 
 		if (priv->do_set_bittiming) {
@@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
 		/* Do not allow changing bittiming while running */
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
+
+		/* Calculate bittiming parameters based on
+		 * data_bittiming_const if set, otherwise pass bitrate
+		 * directly via do_set_bitrate(). Bail out if neither
+		 * is given.
+		 */
+		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
+			return -EOPNOTSUPP;
+
 		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
 		       sizeof(dbt));
-		err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
-		if (err)
-			return err;
+		if (priv->data_bittiming_const) {
+			err = can_get_bittiming(dev, &dbt,
+						priv->data_bittiming_const);
+			if (err)
+				return err;
+		}
 		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
 
 		if (priv->do_set_data_bittiming) {
-- 
2.11.0

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Oliver Hartkopp]

Hi Marc,

On 01/06/2017 01:53 PM, Marc Kleine-Budde wrote:

> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>  		/* Do not allow changing bittiming while running */
>  		if (dev->flags & IFF_UP)
>  			return -EBUSY;
> +
> +		/* Calculate bittiming parameters based on
> +		 * bittiming_const if set, otherwise pass bitrate
> +		 * directly via do_set_bitrate(). Bail out if neither
> +		 * is given.
> +		 */
> +		if (!priv->bittiming_const && !priv->do_set_bittiming)
> +			return -EOPNOTSUPP;
> +

I would suggest to remove this check and the comment here and better add 
an else path with

	return -EOPNOTSUPP;
at
	if (priv->do_set_bittiming) { } else ...


>  		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> -		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
> -		if (err)
> -			return err;
> +		if (priv->bittiming_const) {
> +			err = can_get_bittiming(dev, &bt,
> +						priv->bittiming_const);
> +			if (err)
> +				return err;
> +		}
>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
>
>  		if (priv->do_set_bittiming) {
> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>  		/* Do not allow changing bittiming while running */
>  		if (dev->flags & IFF_UP)
>  			return -EBUSY;
> +
> +		/* Calculate bittiming parameters based on
> +		 * data_bittiming_const if set, otherwise pass bitrate
> +		 * directly via do_set_bitrate(). Bail out if neither
> +		 * is given.
> +		 */
> +		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
> +			return -EOPNOTSUPP;
> +
>  		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>  		       sizeof(dbt));
> -		err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
> -		if (err)
> -			return err;
> +		if (priv->data_bittiming_const) {
> +			err = can_get_bittiming(dev, &dbt,
> +						priv->data_bittiming_const);
> +			if (err)
> +				return err;
> +		}
>  		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>
>  		if (priv->do_set_data_bittiming) {
>

dito here.

Maybe this leads to two patches then:

1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
2. Allow to set bitrates without providing bittiming_const

Regards,
Oliver

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4088 bytes --]

On 01/07/2017 02:22 PM, Oliver Hartkopp wrote:
> Hi Marc,
> 
> On 01/06/2017 01:53 PM, Marc Kleine-Budde wrote:
> 
>> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>>  		/* Do not allow changing bittiming while running */
>>  		if (dev->flags & IFF_UP)
>>  			return -EBUSY;
>> +
>> +		/* Calculate bittiming parameters based on
>> +		 * bittiming_const if set, otherwise pass bitrate
>> +		 * directly via do_set_bitrate(). Bail out if neither
>> +		 * is given.
>> +		 */
>> +		if (!priv->bittiming_const && !priv->do_set_bittiming)
>> +			return -EOPNOTSUPP;
>> +
> 
> I would suggest to remove this check and the comment here and better add 
> an else path with
> 
> 	return -EOPNOTSUPP;
> at
> 	if (priv->do_set_bittiming) { } else ...

Then the code looks like this:

> 	if (data[IFLA_CAN_BITTIMING]) {
> 		struct can_bittiming bt;
> 
> 		/* Do not allow changing bittiming while running */
> 		if (dev->flags & IFF_UP)
> 			return -EBUSY;
> 
> 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> 		if (priv->bittiming_const) {
> 			err = can_get_bittiming(dev, &bt,
> 						priv->bittiming_const);
> 			if (err)
> 				return err;
> 		}
> 		memcpy(&priv->bittiming, &bt, sizeof(bt));
> 
> 		if (priv->do_set_bittiming) {
> 			/* Finally, set the bit-timing registers */
> 			err = priv->do_set_bittiming(dev);
> 			if (err)
> 				return err;
> 		} else if (!priv->bittiming_const) {

This "else" means an implicit "!priv->do_set_bittiming &&" which makes
the code IMHO quite hard to understand.

> 			/* Calculate bittiming parameters based on
> 			 * bittiming_const if set, otherwise pass bitrate
> 			 * directly via do_set_bitrate(). Bail out if neither
> 			 * is given.
> 			 */
> 			return -EOPNOTSUPP;
> 		}
> 	}


>>  		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>> -		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>> -		if (err)
>> -			return err;
>> +		if (priv->bittiming_const) {
>> +			err = can_get_bittiming(dev, &bt,
>> +						priv->bittiming_const);
>> +			if (err)
>> +				return err;
>> +		}
>>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>>  		if (priv->do_set_bittiming) {
>> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>>  		/* Do not allow changing bittiming while running */
>>  		if (dev->flags & IFF_UP)
>>  			return -EBUSY;
>> +
>> +		/* Calculate bittiming parameters based on
>> +		 * data_bittiming_const if set, otherwise pass bitrate
>> +		 * directly via do_set_bitrate(). Bail out if neither
>> +		 * is given.
>> +		 */
>> +		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
>> +			return -EOPNOTSUPP;
>> +
>>  		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>>  		       sizeof(dbt));
>> -		err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
>> -		if (err)
>> -			return err;
>> +		if (priv->data_bittiming_const) {
>> +			err = can_get_bittiming(dev, &dbt,
>> +						priv->data_bittiming_const);
>> +			if (err)
>> +				return err;
>> +		}
>>  		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>
>>  		if (priv->do_set_data_bittiming) {
>>
> 
> dito here.
> 
> Maybe this leads to two patches then:
> 
> 1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming

do_set_bittiming is and was not mandatory. Before your patch it was
mandatory to have one (bittiming_const or do_set_bittiming) or both.
With your patch bittiming_const got mandatory, too. My patch
re-introduces the "bittiming_const or do_set_bittiming" state, which is
directly reflected by the "if (!priv->bittiming_const &&
!priv->do_set_bittiming)" statement.

> 2. Allow to set bitrates without providing bittiming_const

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: 488 bytes --]

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Oliver Hartkopp]

On 01/09/2017 11:39 AM, Marc Kleine-Budde wrote:
> On 01/07/2017 02:22 PM, Oliver Hartkopp wrote:
>> Hi Marc,
>>
>> On 01/06/2017 01:53 PM, Marc Kleine-Budde wrote:
>>
>>> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>>>  		/* Do not allow changing bittiming while running */
>>>  		if (dev->flags & IFF_UP)
>>>  			return -EBUSY;
>>> +
>>> +		/* Calculate bittiming parameters based on
>>> +		 * bittiming_const if set, otherwise pass bitrate
>>> +		 * directly via do_set_bitrate(). Bail out if neither
>>> +		 * is given.
>>> +		 */
>>> +		if (!priv->bittiming_const && !priv->do_set_bittiming)
>>> +			return -EOPNOTSUPP;
>>> +
>>
>> I would suggest to remove this check and the comment here and better add
>> an else path with
>>
>> 	return -EOPNOTSUPP;
>> at
>> 	if (priv->do_set_bittiming) { } else ...
>
> Then the code looks like this:
>
>> 	if (data[IFLA_CAN_BITTIMING]) {
>> 		struct can_bittiming bt;
>>
>> 		/* Do not allow changing bittiming while running */
>> 		if (dev->flags & IFF_UP)
>> 			return -EBUSY;
>>
>> 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>> 		if (priv->bittiming_const) {
>> 			err = can_get_bittiming(dev, &bt,
>> 						priv->bittiming_const);
>> 			if (err)
>> 				return err;
>> 		}
>> 		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> 		if (priv->do_set_bittiming) {
>> 			/* Finally, set the bit-timing registers */
>> 			err = priv->do_set_bittiming(dev);
>> 			if (err)
>> 				return err;
>> 		} else if (!priv->bittiming_const) {
>
> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
> the code IMHO quite hard to understand.

No no no.

I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).

As you suggested in the answer to my RFC these checks for existing 
functions can be moved to the beginning of each code section.

>
>> 			/* Calculate bittiming parameters based on
>> 			 * bittiming_const if set, otherwise pass bitrate
>> 			 * directly via do_set_bitrate(). Bail out if neither
>> 			 * is given.
>> 			 */
>> 			return -EOPNOTSUPP;
>> 		}
>> 	}
>
>
>>>  		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>> -		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>>> -		if (err)
>>> -			return err;
>>> +		if (priv->bittiming_const) {
>>> +			err = can_get_bittiming(dev, &bt,
>>> +						priv->bittiming_const);
>>> +			if (err)
>>> +				return err;
>>> +		}
>>>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>
>>>  		if (priv->do_set_bittiming) {
>>> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>>>  		/* Do not allow changing bittiming while running */
>>>  		if (dev->flags & IFF_UP)
>>>  			return -EBUSY;
>>> +
>>> +		/* Calculate bittiming parameters based on
>>> +		 * data_bittiming_const if set, otherwise pass bitrate
>>> +		 * directly via do_set_bitrate(). Bail out if neither
>>> +		 * is given.
>>> +		 */
>>> +		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
>>> +			return -EOPNOTSUPP;
>>> +
>>>  		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>>>  		       sizeof(dbt));
>>> -		err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
>>> -		if (err)
>>> -			return err;
>>> +		if (priv->data_bittiming_const) {
>>> +			err = can_get_bittiming(dev, &dbt,
>>> +						priv->data_bittiming_const);
>>> +			if (err)
>>> +				return err;
>>> +		}
>>>  		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>>
>>>  		if (priv->do_set_data_bittiming) {
>>>
>>
>> dito here.
>>
>> Maybe this leads to two patches then:
>>
>> 1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
>
> do_set_bittiming is and was not mandatory.

This is broken - isn't it?
Does it make sense to make do_set_bittiming optional?

I would think do_set_bittiming is mandatory but bittiming_const can be 
optional to support a fixed set of bitrates e.g. for the mcba_usb driver?!?

Regards,
Oliver

> Before your patch it was
> mandatory to have one (bittiming_const or do_set_bittiming) or both.
> With your patch bittiming_const got mandatory, too. My patch
> re-introduces the "bittiming_const or do_set_bittiming" state, which is
> directly reflected by the "if (!priv->bittiming_const &&
> !priv->do_set_bittiming)" statement.
>
>> 2. Allow to set bitrates without providing bittiming_const
>
> Marc
>

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 5813 bytes --]

On 01/09/2017 06:23 PM, Oliver Hartkopp wrote:
>>>> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>>>>  		/* Do not allow changing bittiming while running */
>>>>  		if (dev->flags & IFF_UP)
>>>>  			return -EBUSY;
>>>> +
>>>> +		/* Calculate bittiming parameters based on
>>>> +		 * bittiming_const if set, otherwise pass bitrate
>>>> +		 * directly via do_set_bitrate(). Bail out if neither
>>>> +		 * is given.
>>>> +		 */
>>>> +		if (!priv->bittiming_const && !priv->do_set_bittiming)
>>>> +			return -EOPNOTSUPP;
>>>> +
>>>
>>> I would suggest to remove this check and the comment here and better add
>>> an else path with
>>>
>>> 	return -EOPNOTSUPP;
>>> at
>>> 	if (priv->do_set_bittiming) { } else ...
>>
>> Then the code looks like this:
>>
>>> 	if (data[IFLA_CAN_BITTIMING]) {
>>> 		struct can_bittiming bt;
>>>
>>> 		/* Do not allow changing bittiming while running */
>>> 		if (dev->flags & IFF_UP)
>>> 			return -EBUSY;
>>>
>>> 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>> 		if (priv->bittiming_const) {
>>> 			err = can_get_bittiming(dev, &bt,
>>> 						priv->bittiming_const);
>>> 			if (err)
>>> 				return err;
>>> 		}
>>> 		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>
>>> 		if (priv->do_set_bittiming) {
>>> 			/* Finally, set the bit-timing registers */
>>> 			err = priv->do_set_bittiming(dev);
>>> 			if (err)
>>> 				return err;
>>> 		} else if (!priv->bittiming_const) {
>>
>> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
>> the code IMHO quite hard to understand.
> 
> No no no.
> 
> I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).

No, do_set_bittiming is not mandatory.

On SoC internal CAN cores it's easier to program the bittiming
parameters into the controller during open(). As long as the driver sets
the bittiming_const the framework will check if the bittiming parameters
are correct.

> As you suggested in the answer to my RFC these checks for existing 
> functions can be moved to the beginning of each code section.

That's what my original patch proposes, here the resulting code of my patch:

> 	if (data[IFLA_CAN_BITTIMING]) {
> 		struct can_bittiming bt;
> 
> 		/* Do not allow changing bittiming while running */
> 		if (dev->flags & IFF_UP)
> 			return -EBUSY;
> 
> 		/* Calculate bittiming parameters based on
> 		 * bittiming_const if set, otherwise pass bitrate
> 		 * directly via do_set_bitrate(). Bail out if neither
> 		 * is given.
> 		 */
> 		if (!priv->bittiming_const && !priv->do_set_bittiming)
> 			return -EOPNOTSUPP;
> 
> 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> 		if (priv->bittiming_const) {
> 			err = can_get_bittiming(dev, &bt,
> 						priv->bittiming_const);
> 			if (err)
> 				return err;
> 		}
> 		memcpy(&priv->bittiming, &bt, sizeof(bt));
> 
> 		if (priv->do_set_bittiming) {
> 			/* Finally, set the bit-timing registers */
> 			err = priv->do_set_bittiming(dev);
> 			if (err)
> 				return err;
> 		}
> 	}

>>> 			/* Calculate bittiming parameters based on
>>> 			 * bittiming_const if set, otherwise pass bitrate
>>> 			 * directly via do_set_bitrate(). Bail out if neither
>>> 			 * is given.
>>> 			 */
>>> 			return -EOPNOTSUPP;
>>> 		}
>>> 	}
>>
>>
>>>>  		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>>> -		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>>>> -		if (err)
>>>> -			return err;
>>>> +		if (priv->bittiming_const) {
>>>> +			err = can_get_bittiming(dev, &bt,
>>>> +						priv->bittiming_const);
>>>> +			if (err)
>>>> +				return err;
>>>> +		}
>>>>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>>
>>>>  		if (priv->do_set_bittiming) {
>>>> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>>>>  		/* Do not allow changing bittiming while running */
>>>>  		if (dev->flags & IFF_UP)
>>>>  			return -EBUSY;
>>>> +
>>>> +		/* Calculate bittiming parameters based on
>>>> +		 * data_bittiming_const if set, otherwise pass bitrate
>>>> +		 * directly via do_set_bitrate(). Bail out if neither
>>>> +		 * is given.
>>>> +		 */
>>>> +		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
>>>> +			return -EOPNOTSUPP;
>>>> +
>>>>  		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>>>>  		       sizeof(dbt));
>>>> -		err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
>>>> -		if (err)
>>>> -			return err;
>>>> +		if (priv->data_bittiming_const) {
>>>> +			err = can_get_bittiming(dev, &dbt,
>>>> +						priv->data_bittiming_const);
>>>> +			if (err)
>>>> +				return err;
>>>> +		}
>>>>  		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>>>
>>>>  		if (priv->do_set_data_bittiming) {
>>>>
>>>
>>> dito here.
>>>
>>> Maybe this leads to two patches then:
>>>
>>> 1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
>>
>> do_set_bittiming is and was not mandatory.
> 
> This is broken - isn't it?
> Does it make sense to make do_set_bittiming optional?

No, it's perfectly fine, see above.

> I would think do_set_bittiming is mandatory but bittiming_const can be 
> optional to support a fixed set of bitrates e.g. for the mcba_usb driver?!?

Until your patch ("08da7da41ea4 can: provide a separate bittiming_const
parameter to bittiming functions) bittiming_const was optional.

Recap: do_set_bittiming or bittiming_const (or both) must be provided.

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: 488 bytes --]

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Kołłątaj, Remigiusz]

Hi,

I can see three major problems with setting up predefined bitrates:
1. I think that there is no way to pass supported bitrates to
userspace (other than printk)
2. The bitrate that is passed to the driver is calculated/validated by
can_get_bittiming. This causes that driver not always get exact value
that was input by a user.
3. When CAN device is using predefined bitrates you do not have
influence or even access to bittiming parameters. As you noticed I had
to hardcode those values in mcba_usb to present them correctly to
userspace.

Based on solution created by Oliver for termination I created
something similar for predefined bitrates. It solves issue number 1
and 2. Not sure how to solve issue 3 correctly to maintain
compatibility (adding another netlink value for predefined bitrate
will break it). As for now I just zeroed values.

The patch is just to visualise my idea. It definitely needs some more work.

Inside the driver:
const u32 predef_bitrate[] = {20000, 50000, 100000};

priv->bitrate_const = predef_bitrate;
priv->bitrate_const_cnt = ARRAY_SIZE(predef_bitrate);

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8d6208c0b400..d2034a69c990 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -279,25 +279,44 @@ static int can_fixup_bittiming(struct net_device
*dev, struct can_bittiming *bt,
        return 0;
 }

+/*
+ * Checks the validity of predefined bitrate settings
+ */
+static int can_validate_bittiming(struct net_device *dev, struct
can_bittiming *bt)
+{
+       struct can_priv *priv = netdev_priv(dev);
+       int i;
+
+       for(i = 0; i < priv->bitrate_const_cnt &&
+               bt->bitrate != priv->bitrate_const[i]; ++i);
+
+       if (i >= priv->bitrate_const_cnt)
+               return -EINVAL;
+
+       /* Clear bittiming settings to idicate we are using predefined
bitrate */
+       memset(bt, 0, sizeof(struct can_bittiming));
+       bt->bitrate = priv->bitrate_const[i];
+
+       return 0;
+}
+
 static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
                             const struct can_bittiming_const *btc)
 {
        int err;

-       /* Check if the CAN device has bit-timing parameters */
-       if (!btc)
-               return -EOPNOTSUPP;
-
        /*
         * Depending on the given can_bittiming parameter structure the CAN
         * timing parameters are calculated based on the provided bitrate OR
         * alternatively the CAN timing parameters (tq, prop_seg, etc.) are
         * provided directly which are then checked and fixed up.
         */
-       if (!bt->tq && bt->bitrate)
+       if (!bt->tq && bt->bitrate && btc)
                err = can_calc_bittiming(dev, bt, btc);
-       else if (bt->tq && !bt->bitrate)
+       else if (bt->tq && !bt->bitrate && btc)
                err = can_fixup_bittiming(dev, bt, btc);
+       else if (bt->bitrate && !btc)
+               err = can_validate_bittiming(dev, bt);
        else
                err = -EINVAL;

@@ -1018,7 +1037,11 @@ static int can_fill_info(struct sk_buff *skb,
const struct net_device *dev)
            (priv->data_bittiming_const &&
             nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
                     sizeof(*priv->data_bittiming_const),
-                    priv->data_bittiming_const)))
+                    priv->data_bittiming_const)) ||
+
+           (priv->bitrate_const &&
+            nla_put(skb, IFLA_CAN_BITRATE_CONST,
+                    sizeof(u32) * priv->bitrate_const_cnt,
priv->bitrate_const)))
                return -EMSGSIZE;

        return 0;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5f5270941ba0..fa5e8337f0e3 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -38,6 +38,8 @@ struct can_priv {
        struct can_bittiming bittiming, data_bittiming;
        const struct can_bittiming_const *bittiming_const,
                *data_bittiming_const;
+       const u32 *bitrate_const;
+       const int bitrate_const_cnt;
        struct can_clock clock;

        enum can_state state;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 94ffe0c83ce7..7d1815dad72b 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -127,6 +127,7 @@ enum {
        IFLA_CAN_BERR_COUNTER,
        IFLA_CAN_DATA_BITTIMING,
        IFLA_CAN_DATA_BITTIMING_CONST,
+       IFLA_CAN_BITRATE_CONST,
        __IFLA_CAN_MAX
 };



Regards,
Remik

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1763 bytes --]

On 01/10/2017 11:18 AM, Kołłątaj, Remigiusz wrote:
> Hi,
> 
> I can see three major problems with setting up predefined bitrates:
> 1. I think that there is no way to pass supported bitrates to
> userspace (other than printk)

yes

> 2. The bitrate that is passed to the driver is calculated/validated by
> can_get_bittiming. This causes that driver not always get exact value
> that was input by a user.

Have you seen my patch?

> 3. When CAN device is using predefined bitrates you do not have
> influence or even access to bittiming parameters. As you noticed I had
> to hardcode those values in mcba_usb to present them correctly to
> userspace.

With my patch, there's no need to set bittiming_const anymore.

> Based on solution created by Oliver for termination I created
> something similar for predefined bitrates. It solves issue number 1
> and 2. Not sure how to solve issue 3 correctly to maintain
> compatibility (adding another netlink value for predefined bitrate
> will break it). As for now I just zeroed values.
> 
> The patch is just to visualise my idea. It definitely needs some more work.
> 
> Inside the driver:
> const u32 predef_bitrate[] = {20000, 50000, 100000};
> 
> priv->bitrate_const = predef_bitrate;
> priv->bitrate_const_cnt = ARRAY_SIZE(predef_bitrate);

The general idea makes sense, although we need support for CANFD, too.
Let's first agree on Oliver's and my patch, then yours will fit in easily.

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: 484 bytes --]

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Kołłątaj, Remigiusz]

Marc,

On 10 January 2017 at 11:33, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 01/10/2017 11:18 AM, Kołłątaj, Remigiusz wrote:
>> Hi,
>>
>> I can see three major problems with setting up predefined bitrates:
>> 1. I think that there is no way to pass supported bitrates to
>> userspace (other than printk)
>
> yes
>
>> 2. The bitrate that is passed to the driver is calculated/validated by
>> can_get_bittiming. This causes that driver not always get exact value
>> that was input by a user.
>
> Have you seen my patch?
Yes, your patch solves that issue too.

>
>> 3. When CAN device is using predefined bitrates you do not have
>> influence or even access to bittiming parameters. As you noticed I had
>> to hardcode those values in mcba_usb to present them correctly to
>> userspace.
>
> With my patch, there's no need to set bittiming_const anymore.
You are right, but can_bittiming is still passed to userspace and
since devices with predefined bitrates have no interest in bittiming
parameters the struct will contain undefined values (i.e. tq,
prop_seg, phase_seg etc.).

>
>> Based on solution created by Oliver for termination I created
>> something similar for predefined bitrates. It solves issue number 1
>> and 2. Not sure how to solve issue 3 correctly to maintain
>> compatibility (adding another netlink value for predefined bitrate
>> will break it). As for now I just zeroed values.
>>
>> The patch is just to visualise my idea. It definitely needs some more work.
>>
>> Inside the driver:
>> const u32 predef_bitrate[] = {20000, 50000, 100000};
>>
>> priv->bitrate_const = predef_bitrate;
>> priv->bitrate_const_cnt = ARRAY_SIZE(predef_bitrate);
>
> The general idea makes sense, although we need support for CANFD, too.
I'm not so familiar with CANFD. Do you mean can_data_bittiming part?

> Let's first agree on Oliver's and my patch, then yours will fit in easily.
>
Ok.

Cheers,
Remik

-- 
____________________
Remigiusz Kołłątaj
Mobica Ltd
Technology Consultant
Skype: remigiusz.kollataj
www.mobica.com

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Oliver Hartkopp]

On 01/10/2017 09:39 AM, Marc Kleine-Budde wrote:

>>> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
>>> the code IMHO quite hard to understand.
>>
>> No no no.
>>
>> I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).
>
> No, do_set_bittiming is not mandatory.
>
> On SoC internal CAN cores it's easier to program the bittiming
> parameters into the controller during open(). As long as the driver sets
> the bittiming_const the framework will check if the bittiming parameters
> are correct.

Oh. Didn't know that use-case for SoCs.

>> As you suggested in the answer to my RFC these checks for existing
>> functions can be moved to the beginning of each code section.
>
> That's what my original patch proposes, here the resulting code of my patch:
>
>> 	if (data[IFLA_CAN_BITTIMING]) {
>> 		struct can_bittiming bt;
>>
>> 		/* Do not allow changing bittiming while running */
>> 		if (dev->flags & IFF_UP)
>> 			return -EBUSY;
>>
>> 		/* Calculate bittiming parameters based on
>> 		 * bittiming_const if set, otherwise pass bitrate
>> 		 * directly via do_set_bitrate(). Bail out if neither
>> 		 * is given.
>> 		 */
>> 		if (!priv->bittiming_const && !priv->do_set_bittiming)
>> 			return -EOPNOTSUPP;
>>
>> 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>> 		if (priv->bittiming_const) {
>> 			err = can_get_bittiming(dev, &bt,
>> 						priv->bittiming_const);
>> 			if (err)
>> 				return err;
>> 		}
>> 		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> 		if (priv->do_set_bittiming) {

I was stumbling upon this additional check.

But now it seems correct to me ;-)

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks,
Oliver

Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2141 bytes --]

On 01/10/2017 05:54 PM, Oliver Hartkopp wrote:
> On 01/10/2017 09:39 AM, Marc Kleine-Budde wrote:
> 
>>>> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
>>>> the code IMHO quite hard to understand.
>>>
>>> No no no.
>>>
>>> I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).
>>
>> No, do_set_bittiming is not mandatory.
>>
>> On SoC internal CAN cores it's easier to program the bittiming
>> parameters into the controller during open(). As long as the driver sets
>> the bittiming_const the framework will check if the bittiming parameters
>> are correct.
> 
> Oh. Didn't know that use-case for SoCs.
> 
>>> As you suggested in the answer to my RFC these checks for existing
>>> functions can be moved to the beginning of each code section.
>>
>> That's what my original patch proposes, here the resulting code of my patch:
>>
>>> 	if (data[IFLA_CAN_BITTIMING]) {
>>> 		struct can_bittiming bt;
>>>
>>> 		/* Do not allow changing bittiming while running */
>>> 		if (dev->flags & IFF_UP)
>>> 			return -EBUSY;
>>>
>>> 		/* Calculate bittiming parameters based on
>>> 		 * bittiming_const if set, otherwise pass bitrate
>>> 		 * directly via do_set_bitrate(). Bail out if neither
>>> 		 * is given.
>>> 		 */
>>> 		if (!priv->bittiming_const && !priv->do_set_bittiming)
>>> 			return -EOPNOTSUPP;
>>>
>>> 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>> 		if (priv->bittiming_const) {
>>> 			err = can_get_bittiming(dev, &bt,
>>> 						priv->bittiming_const);
>>> 			if (err)
>>> 				return err;
>>> 		}
>>> 		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>
>>> 		if (priv->do_set_bittiming) {
> 
> I was stumbling upon this additional check.
> 
> But now it seems correct to me ;-)
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

tnx.

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: 488 bytes --]