netdev - Re: [PATCH v5 8/8] can-dev: add len8_dlc support for various CAN USB adapters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Nov 2020 08:46:32 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Oliver Hartkopp <socketcan@...tkopp.net>,
        linux-can@...r.kernel.org, mailhol.vincent@...adoo.fr
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v5 8/8] can-dev: add len8_dlc support for various CAN USB
 adapters

On 11/10/20 7:55 AM, Oliver Hartkopp wrote:
>>> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
>>> index 940589667a7f..cc0c30a33335 100644
>>> --- a/drivers/net/can/usb/gs_usb.c
>>> +++ b/drivers/net/can/usb/gs_usb.c
>>> @@ -330,10 +330,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>>>   			return;
>>>   
>>>   		cf->can_id = hf->can_id;
>>>   
>>>   		cf->len = can_cc_dlc2len(hf->len);
>>> +		cf->len8_dlc = can_get_len8_dlc(dev->can.ctrlmode, cf->len,
>>> +						hf->len);
>>
>> What about introducing a function that sets len and len8_dlc at the same time:
>>
>> void can_frame_set_length(const struct can_priv *can, struct can_frame *cfd, u8
>> dlc);
> 
> Good idea.
> 
> I would suggest something like
> 
> u8 can_get_cc_len(const u32 ctrlmode, struct can_frame *cf, u8 dlc)
> 
> that still returns the 'len' element, so that we can replace 
> can_cc_dlc2len() with can_get_cc_len() for CAN drivers that add support 
> for len8_dlc.

The regex to replace can_cc_dlc2len() with can_get_cc_len() might be simpler,
but passing the cf by reference _and_ assigning the return value to a member of
cf looks strange.

> The assignment cf->len = can_get_cc_len() fits better into the code 
> which assigns cf->can_id too.
> 
> And I would stay on 'u32 ctrlmode' as ctrlmode is the parameter which is 
> namely needed here. A pointer to can_priv can mean anything.

OK
>> And maybe a function that takes a canfd_frame, so that we don't need to cast....
> 
> No. The len8_dlc element is from struct can_frame. When people use the 
> struct canfd_frame in their driver this might have some benefits for them.
> But when it comes to access the len8_dlc element this has to be casted IMO.
> 
> But with the suggested can_get_cc_len() function a needed cast could be 
> put into the parameter list without adding extra code somewhere else in 
> the driver.

OK

Marc

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



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)