Re: [PATCH 4/4] can: c_can: Add d_can suspend resume support

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

On 09/13/2012 09:24 AM, AnilKumar, Chimata wrote:
> Marc,
> 
> On Wed, Sep 12, 2012 at 18:32:53, Marc Kleine-Budde wrote:
>> On 09/12/2012 02:48 PM, AnilKumar, Chimata wrote:
>>> Hi Marc,
>>>
>>> On Tue, Sep 04, 2012 at 12:57:18, Marc Kleine-Budde wrote:
>>>> On 09/04/2012 08:14 AM, AnilKumar, Chimata wrote:
>>>>> Marc,
>>>>>
>>>>> Thanks for the comments,
>>>>>
>>>>> On Tue, Sep 04, 2012 at 01:31:35, Marc Kleine-Budde wrote:
>>>>>> On 09/03/2012 01:52 PM, AnilKumar Ch wrote:
>>>>>>> Adds suspend resume support to DCAN driver which enables
>>>>>>> DCAN power down mode bit (PDR). Then DCAN will ack the local
>>>>>>> power-down mode by setting PDA bit in STATUS register.
>>>>>>>
>>>>>>> Also adds a status flag to know the status of DCAN module,
>>>>>>> whether it is opened or not.
>>>>>>
>>>>>> Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver
>>>>>> [1]. I'm not sure if you need locking here.
>>>>>>
>>>>>
>>>>> Then I can use this to check the status, lock is not
>>>>> required.
>>>>>
>>>>>> [1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198
>>>>>>
>>>>>>> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
>>>>>>> ---
>>>>>>>  drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
>>>>>>>  drivers/net/can/c_can/c_can.h          |    5 ++
>>>>>>>  drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
>>>>>>>  3 files changed, 150 insertions(+), 3 deletions(-)
>>>
>>> [snip]
>>>
>>>>>>> +#ifdef CONFIG_PM
>>>>>>> +int c_can_power_down(struct net_device *dev)
>>>>>>> +{
>>>>>>> +	unsigned long time_out;
>>>>>>> +	struct c_can_priv *priv = netdev_priv(dev);
>>>>>>> +
>>>>>>> +	if (!priv->is_opened)
>>>>>>> +		return 0;
>>>>>>
>>>>>> Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?
>>>>>
>>>>> These APIs are called from platform driver where device type
>>>>> device type is verified. I think we have to add BUG_ON() in
>>>>> platform driver.
>>>>
>>>> The platform driver returns if not on D_CAN and will not call this
>>>> function. But this functions are exported, so can be called by someone
>>>> else. So if the caller is not D_CAN, it's a bug.
>>>>
>>>
>>> I agree with you, but I have some concern here.
>>> I think we should do "return 0;" instead of BUG_ON(). With
>>> BUG_ON() system will hang and ultimately user lost his/her
>>> contents.
>>
>> Good point, better safe then sorry.
>> But this should not happen. What about WARN_ON()?
>>
> 
> Either would be fine printing a warning message or WARN_ON()

I'm for a  WARN_ON()

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