netdev - Re: [PATCH net-next-2.6 v4 1/1] can: c_can: Added support for Bosch C

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] [day] [month] [year] [list]

Date:	Wed, 12 Jan 2011 10:30:02 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Wolfgang Grandegger <wg@...ndegger.com>
CC:	Bhupesh SHARMA <bhupesh.sharma@...com>,
	"Socketcan-core@...ts.berlios.de" <Socketcan-core@...ts.berlios.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next-2.6 v4 1/1] can: c_can: Added support for Bosch
 C_CAN controller

On 01/12/2011 10:08 AM, Wolfgang Grandegger wrote:
> On 01/12/2011 09:51 AM, Bhupesh SHARMA wrote:
>> Hi Marc,
>>
>> Thanks for your review.
>> Please see my comments inline:
>>
>>> -----Original Message-----
>>> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
>>> On 01/11/2011 12:49 PM, Bhupesh Sharma wrote:
> ...
>>>> +static int c_can_close(struct net_device *dev) {
>>>> +   struct c_can_priv *priv = netdev_priv(dev);
>>>> +
>>>> +   netif_stop_queue(dev);
>>>> +   napi_disable(&priv->napi);
>>>> +   c_can_stop(dev);
>>>> +   free_irq(dev->irq, dev);
>>>> +   close_candev(dev);
>>>> +
>>>> +   return 0;
>>>> +}
>>>> +
>>>> +struct net_device *alloc_c_can_dev(void)
>>>
>>> Please model after alloc_sja1000_dev:
>>>
>>> struct net_device *alloc_sja1000dev(int sizeof_priv)
>>>
>>> The private for the _user_ of alloc_c_can_dev is behind the sja1000
>>> private, so you can get rid of the void *priv member in the struct
>>> c_can_priv. (see below)
>>
>> Ok.
>> But Wolfgang also suggested to use the *priv* member inside c_can_priv struct
>> for board specific details. In my c_can platform driver I use it to
>> store the *clk* variable. Do I need to change that as well?
> 
> Marc is referring to:
> 
> http://lxr.linux.no/#linux+v2.6.37/drivers/net/can/sja1000/sja1000.c#L582
> 
> But also there "priv->priv" is used to store a pointer to the board
> specific details. Looking to the SJA1000 drivers, only *one* uses
> "sizeof_priv > 0", but many other attach a separately allocated
> structure to "priv->priv". For that reason, I'm fine with your current
> implementation.

Okay fine with me.

A nice cleanup might be to introduce something like this:

static inline void * give_me_my_priv_from_sja1000_priv
	(struct sja1000_priv *priv)
{
	return (void *)(priv + 1);
}

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   |


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