On 12/19/19 2:37 PM, Grygorii Strashko wrote:
>
>
> On 17/12/2019 12:55, Marek Vasut wrote:
>> On 12/17/19 11:42 AM, Grygorii Strashko wrote:
>>>
>>>
>>> On 17/12/2019 12:07, Marek Vasut wrote:
>>>> The current code causes a slight glitch on the pinctrl settings when
>>>> used.
>>>> Since commit ab78029 (drivers/pinctrl: grab default handles from
>>>> device core),
>>>> the device core will automatically set the default pins. This causes
>>>> the pins
>>>> to be momentarily set to the default and then to the sleep state in
>>>> register_m_can_dev(). By adding an optional "enable" state, boards can
>>>> set the
>>>> default pin state to be disabled and avoid the glitch when the switch
>>>> from
>>>> default to sleep first occurs. If the "enable" state is not available
>>>> pinctrl_get_select() falls back to using the "default" pinctrl state.
>>>>
>>>> Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each
>>>> suspend/resume function")
>>>> Signed-off-by: Marek Vasut <marex@xxxxxxx>
>>>> Cc: Bich Hemon <bich.hemon@xxxxxx>
>>>> Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
>>>> Cc: J.D. Schroeder <jay.schroeder@xxxxxxxxxx>
>>>> Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
>>>> Cc: Roger Quadros <rogerq@xxxxxx>
>>>> Cc: linux-stable <stable@xxxxxxxxxxxxxxx>
>>>> To: linux-can@xxxxxxxxxxxxxxx
>>>> ---
>>>> NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux
>>>> glitch at init")
>>>> adapted for m_can driver.
>>>> ---
>>>> drivers/net/can/m_can/m_can.c | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/net/can/m_can/m_can.c
>>>> b/drivers/net/can/m_can/m_can.c
>>>> index 02c5795b73936..afb6760b17427 100644
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct
>>>> net_device *dev)
>>>> static void m_can_start(struct net_device *dev)
>>>> {
>>>> struct m_can_classdev *cdev = netdev_priv(dev);
>>>> + struct pinctrl *p;
>>>> /* basic m_can configuration */
>>>> m_can_chip_config(dev);
>>>> cdev->can.state = CAN_STATE_ERROR_ACTIVE;
>>>> + /* Attempt to use "active" if available else use "default" */
>>>> + p = pinctrl_get_select(cdev->dev, "active");
>>>> + if (!IS_ERR(p))
>>>> + pinctrl_put(p);
>>>> + else
>>>> + pinctrl_pm_select_default_state(cdev->dev);
>>>> +
>>>> m_can_enable_all_interrupts(cdev);
>>>> }
>>>>
>>>
>>> May be init state should be used - #define PINCTRL_STATE_INIT "init"
>>> instead?
>>
>> I'm not sure I quite understand -- how ?
>>
>
> Sry, for delayed reply.
>
> I've looked at m_can code and think issue is a little bit deeper
> (but I might be wrong as i'm not can expert and below based on code
> review).
>
> First, what going on:
> probe:
> really_probe()
> pinctrl_bind_pins()
> if (IS_ERR(dev->pins->init_state)) {
> ret = pinctrl_select_state(dev->pins->p,
> dev->pins->default_state);
> } else {
> ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
> }
> [GS] So at this point default_state or init_state is set
>
> ret = dev->bus->probe(dev);
> m_can_plat_probe()
> m_can_class_register()
> m_can_clk_start()
> pm_runtime_get_sync()
> m_can_runtime_resume()
> [GS] Still default_state or init_state is active
>
> register_m_can_dev()
> [GS] at this point m_can netdev is registered, which may lead to
> .ndo_open = m_can_open() call
>
> m_can_clk_stop()
> pm_runtime_put_sync()
> [GS] if .ndo_open() was called before it will be a nop
> m_can_runtime_suspend()
> m_can_class_suspend()
>
> if (netif_running(ndev)) {
> netif_stop_queue(ndev);
> netif_device_detach(ndev);
> m_can_stop(ndev);
> m_can_clk_stop(cdev);
> [GS] if .ndo_open() was called before it will lead to deadlock here
> So, most probably, it will cause deadlock in case of "ifconfig
> <m_can_dev> up down" case
> }
>
> pinctrl_pm_select_sleep_state(dev);
> [GS] at this point sleep_state will be set - i assume it's the root
> cause of your glitch.
> Note - As per code, the pinctrl default_state will never ever
> configured again, so if after
> probe m_can will go through PM runtime suspend/resume cycle it
> will not work any more.
>
> pinctrl_init_done()
> [GS] will do nothing in case !init_state
>
> As per above, if sleep_state is defined the m_can seems should not work
> at all without your patch,
> as there is no code path to switch back sleep_state->default_state.
> And over all PM runtime m_can code is mixed with System suspend code and
> so not correct.
>
> Also, the very good question - Is it really required to toggle pinctrl
> states as part of PM runtime?
> (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this
patch is trying to fix ?
