-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Oliver Hartkopp wrote:
> Marc Kleine-Budde wrote:
>> Wolfgang Grandegger wrote:
>>> This patch makes the private functions alloc_can_skb() and
>>> alloc_can_err_skb() of the at91_can driver public and adapts all
>>> drivers to use these. While making the patch I realized, that
>>> the skb's are *not* setup consistently. The skb's are now setup
>>> as shown:
>>> skb->dev = dev;
>> nitpick: we use "netdev_alloc_skb()", so "skb->dev = dev" isn't needed
>> any more. The code is allright, just the commit message is misleading.
>>
>>> skb->protocol = __constant_htons(ETH_P_CAN);
>>> skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> Some drivers or library code used:
>>> skb->protocol = htons(ETH_P_CAN); or
>>> skb->pkt_type = PACKET_BROADCAST;
>>> or did not set CHECKSUM_UNNECESSARY.
>>> Please check and comment.
>>> Marc, feel free to add your signed-off-by here.
>> nitpick: as you did the patch, I can rather add my Acked-by: under your
>> S-o-b.
>>
>>> Signed-off-by: Wolfgang Grandegger <[email protected]>
>>> ---
>>> kernel/2.6/drivers/net/can/at91_can.c | 32 ------------------
>>> kernel/2.6/drivers/net/can/cc770/cc770.c | 13 +------
>>> kernel/2.6/drivers/net/can/dev.c | 38
>>> ++++++++++++++++++++--
>>> kernel/2.6/drivers/net/can/esd_pci331.c | 14 +-------
>>> kernel/2.6/drivers/net/can/mcp251x.c | 20 +----------
>>> kernel/2.6/drivers/net/can/mscan/mscan.c | 6 ---
>>> kernel/2.6/drivers/net/can/sja1000/sja1000.c | 12 +-----
>>> kernel/2.6/drivers/net/can/softing/softing_main.c | 8 +---
>>> kernel/2.6/drivers/net/can/usb/ems_usb.c | 16 +--------
>>> kernel/2.6/include/socketcan/can/dev.h | 4 ++
>>> 10 files changed, 54 insertions(+), 109 deletions(-)
>>> Index: trunk/kernel/2.6/drivers/net/can/dev.c
>>> ===================================================================
>>> --- trunk.orig/kernel/2.6/drivers/net/can/dev.c
>>> +++ trunk/kernel/2.6/drivers/net/can/dev.c
>>> @@ -318,8 +318,7 @@ void can_put_echo_skb(struct sk_buff *sk
>>> skb->sk = srcsk;
>>> /* make settings for echo to reduce code in irq context */
>>> - skb->protocol = htons(ETH_P_CAN);
>>> - skb->pkt_type = PACKET_BROADCAST;
>> is it save to remove this flag?
>
> This was also my concern.
>
> This pkt_type is also set in af_can.c and we should set it for now.
> You might also use CAN netdevices with PACKET socket and wireshark, where the
> PACKET_BROADCAST pkt_type is remarked.
>
> If it is unnecessary i would send a separate patch to remove both of them.
>
>>> + skb->protocol = __constant_htons(ETH_P_CAN);
>>> skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> skb->dev = dev;
>>> @@ -403,7 +402,8 @@ void can_restart(unsigned long data)
>>> goto restart;
>>> }
>>> skb->dev = dev;
>>> - skb->protocol = htons(ETH_P_CAN);
>>> + skb->protocol = __constant_htons(ETH_P_CAN);
>>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>>> memset(cf, 0, sizeof(struct can_frame));
>>> cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
>>> @@ -496,6 +496,38 @@ static void can_setup(struct net_device
>>> #endif
>>> }
>>> +struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame
>>> **cf)
>>> +{
>>> + struct sk_buff *skb;
>>> +
>>> + skb = netdev_alloc_skb(dev, sizeof(struct can_frame));
>>> + if (unlikely(!skb))
>>> + return NULL;
>>> +
>>> + skb->protocol = __constant_htons(ETH_P_CAN);
>>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> Please add 'skb->pkt_type = PACKET_BROADCAST' here.
>
>>> + *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>
> This could be also the place, where we could memset the struct can_frame to
> zero, right?
>
> Maybe we should do it here and leave it in alloc_can_err_skb() ...
In my at91 driver I don't need the memset, because I unconditionally
read the two 32 bit registers and write it into the ->data of the can_frame.
>
>>> +
>>> + return skb;
>>> +}
>>> +EXPORT_SYMBOL_GPL(alloc_can_skb);
>>> +struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame
>>> **cf)
>>> +{
>>> + struct sk_buff *skb;
>>> +
>>> + skb = alloc_can_skb(dev, cf);
>>> + if (unlikely(!skb))
>>> + return NULL;
>>> +
>>> + memset(*cf, 0, sizeof(struct can_frame));
>
> here :-)
Here we need zeroed ->data
>>> + (*cf)->can_id = CAN_ERR_FLAG;
>>> + (*cf)->can_dlc = CAN_ERR_DLC;
>>> +
>>> + return skb;
>>> +}
>>> +EXPORT_SYMBOL_GPL(alloc_can_err_skb);
cheers, Marc
- --
Pengutronix e.K. | Marc Kleine-Budde |
Linux Solutions for Science and Industry | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkrLYTsACgkQjTAFq1RaXHMIigCfR99QYgrwq/oBXiLroX6xzM06
u14An2mYbiySDr11gwHc7k15VuVHwOVM
=haXq
-----END PGP SIGNATURE-----
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
