netdev - Re: [PATCH net-next-2.6 v3 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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 11 Jan 2011 09:29:50 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Bhupesh SHARMA <bhupesh.sharma@...com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Socketcan-core@...ts.berlios.de" <Socketcan-core@...ts.berlios.de>,
	Marc Kleine-Budde <mkl@...gutronix.de>,
	Oliver Hartkopp <socketcan@...tkopp.net>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next-2.6 v3 1/1] can: c_can: Added support for Bosch
 C_CAN controller

Hi Bhupesh,

On 01/11/2011 05:50 AM, Bhupesh SHARMA wrote:
> Hi Wolfgang,
> 
> Thanks for your review.
> Please see my comments inline.
> 
>> -----Original Message-----
>> From: Wolfgang Grandegger [mailto:wg@...ndegger.com]
...

>>> + * Bosch C_CAN controller is compliant to CAN protocol version 2.0
>> part A and B.
>>> + * Bosch C_CAN user manual can be obtained from:
>>> + * http://www.semiconductors.bosch.de/pdf/Users_Manual_C_CAN.pdf
>>
>> Unfortunately, this link is not valid any more.
> 
> Oops..
> It seems they have shifted to:
> http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/c_can/users_manual_c_can.pdf

Ah, nice. I was not aware of that new link.

...
>>> +int c_can_write_msg_object(struct net_device *dev,
>>> +                   int iface, struct can_frame *frame, int objno)
>>> +{
>>> +   int i;
>>> +   u16 flags = 0;
>>> +   unsigned int id;
>>> +   struct c_can_priv *priv = netdev_priv(dev);
>>> +
>>> +   if (frame->can_id & CAN_EFF_FLAG) {
>>> +           id = frame->can_id & CAN_EFF_MASK;
>>> +           flags |= IF_ARB_MSGXTD;
>>> +   } else
>>> +           id = ((frame->can_id & CAN_SFF_MASK) << 18);

I just realize that the  {} for the else block is missing.

>>> +   /*
>>> +    * check for 'last error code' which tells us the
>>> +    * type of the last error to occur on the CAN bus
>>> +    */
>>> +   switch (lec_type) {
>>> +           /* common for all type of bus errors */
>>> +           priv->can.can_stats.bus_error++;
>>> +           stats->rx_errors++;
>>> +           cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +           cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>>
>> Are you sure that this part is ever executed? I wonder why the compile
>> does
>> not complain.
> 
> I did not get any compilation error.

I know.

> But I will check if the program flow enters this part or not.

Good. It was *not* executed in my little user space test program.

...
>>> +static int __devexit c_can_plat_remove(struct platform_device *pdev)
>>> +{
>>> +   struct net_device *dev = platform_get_drvdata(pdev);
>>> +   struct c_can_priv *priv = netdev_priv(dev);
>>> +   struct resource *mem;
>>> +
>>> +   /* disable all interrupts */
>>> +   c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
>>
>> To avoid exportign that function, couldn't it be done at the beginning
>> of
>> unregister_c_can_dev()?
> 
> Yes this can be done.
> But, IMHO *disabling* interrupts seems more logical as part of __devexit
> Code.

I think the interrupts are already disabled when you unload the module
because the device must be closed (c_can_stop has been called). Anyway,
I think the above c_can_enable_all_interrupts() call is well placed in
the unregister function. I would avoid the export the function.

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html