netdev - Re: [PATCH] sh_eth: add wake-on-lan support via magic packet

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:   Mon, 12 Dec 2016 20:35:27 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Niklas Söderlund <niklas.soderlund@...natech.se>
Cc:     Simon Horman <horms+renesas@...ge.net.au>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] sh_eth: add wake-on-lan support via magic packet

On 12/12/2016 06:49 PM, Niklas Söderlund wrote:

> Thanks for your feedback.

    Not at all, it's my duty now. :-)
    I should probably have warned you not to post the new version to netdev -- 
DaveM has closed his net-next.git tree (ahead of the usual time, which would 
have been 4.9 release), so you posting would only upset him...

[...]
>>>>   You only enable the WOL support fo the R-Car gen2 chips but never say that
>>>> explicitly, neither in the subject nor here.
>>>>
>>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
>>>>> ---
>>>>>  drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++---
>>>>>  drivers/net/ethernet/renesas/sh_eth.h |   4 ++
>>>>>  2 files changed, 116 insertions(+), 8 deletions(-)
>>>>
>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
>>>>> index 05b0dc5..3974046 100644
>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
>>>> +		/* Handle MagicPacket interrupt */
>>>> +		if (sh_eth_read(ndev, ECSR) & ECSR_MPD)
>>
>>    What if it wasn't enabled ATM?
>
> Sorry I don't understand this comment.

    I'm trying to handle only the enabled interrupts but this hasn't been 
consistently done yet (only for EESR, not ECSR), so nevermind. :-)

[...]
>>>>> @@ -3150,15 +3193,71 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
[...]

>>> This is how it's done in
>>> other parts of the driver when disabling interrupts.
>>
>>    Not in all parts of the driver that disable EESIPR interrupts... I must
>> confess that I never liked that 'mdp->irq_enabled' flag and still suspect we
>> can get things done without it... I need to look at this code again, sigh...

    Well, we can't most probably but I have a patch almost ready that turns 
the boolean flag into u32 field holding the EESIPR value to be written next. 
Would that help you?

>>> This is also why I only check for MagicPacket interrupts if irq_enabled
>>> is false.
>>
>>   I would have preferred that this was done with the other EMAC interrupts,
>> in sh_eth_error().
>
> I removed the check for Magic Packet in sh_eth_interrupt() and running
> without setting mdp->irq_enabled = false. sh_eth_error() will then clear
> any ECI interrupt so no need to add Magic Packet detection to it since
> all that is needed on Magic Packet is to clear the interrupt which
> already is done. This works and I can do multiple suspend/resume cycles,
> will be in v2 thanks for the suggestion.

    OK, let's see what you have when I have some more time. We have a lot of 
time for ironing things out till net-next is opened again -- which will happen 
after -rc1)...

[...]
>>>>> +
>>>>> +	/* Enable MagicPacket */
>>>>> +	sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
>>>>> +
>>>>> +	/* Increased clock usage so device won't be suspended */
>>>>> +	clk_enable(mdp->clk);
>>>>
>>>>    Hum, intermixiggn runtime PM with clock API doesn't look good...
>>>
>>> I agree it looks weird but I need a way to increment the usage count for
>>> the clock otherwise the PM code will disable the module clock and WoL
>>> will not work.
>>
>>    How will it do it if you don't call sh_eth_close() in this case?
>>
>>> Note that this call will not enable the clock just
>>> increase the usage count so it won't be disabled when the PM code
>>> decrease it after the sh_eth suspend function is run.
>>
>>    You mean that the PM code calls RPM or clk API on its own? That's strange...
>
> Yes it calls clk API.

    Hum, will have to look into it as well...

[...]

MBR, Sergei