netdev - RE: [PATCH V2 3/4] can: flexcan: change the way of stop mode acknowledgment

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:   Wed, 4 Dec 2019 02:23:48 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>,
        "sean@...nix.com" <sean@...nix.com>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
CC:     dl-linux-imx <linux-imx@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH V2 3/4] can: flexcan: change the way of stop mode
 acknowledgment


> -----Original Message-----
> From: Marc Kleine-Budde <mkl@...gutronix.de>
> Sent: 2019年12月4日 2:22
> To: Joakim Zhang <qiangqing.zhang@....com>; sean@...nix.com;
> linux-can@...r.kernel.org
> Cc: dl-linux-imx <linux-imx@....com>; netdev@...r.kernel.org
> Subject: Re: [PATCH V2 3/4] can: flexcan: change the way of stop mode
> acknowledgment
> 
> On 11/27/19 6:56 AM, Joakim Zhang wrote:
> > Stop mode is entered when Stop mode is requested at chip level and
> > MCR[LPM_ACK] is asserted by the FlexCAN.
> >
> > Double check with IP owner, should poll MCR[LPM_ACK] for stop mode
> > acknowledgment, not the acknowledgment from chip level which is used
> > for glitch filter.
> >
> > Fixes: 5f186c257fa4(can: flexcan: fix stop mode acknowledgment)
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> > ------
> > ChangeLog:
> > 	V1->V2: no change.
> > ---
> >  drivers/net/can/flexcan.c | 64
> > ++++++++++++++++++++-------------------
> >  1 file changed, 33 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 5d5ed28d3005..d178146b3da5 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -388,6 +388,34 @@ static struct flexcan_mb __iomem
> *flexcan_get_mb(const struct flexcan_priv *priv
> >  		(&priv->regs->mb[bank][priv->mb_size * mb_index]);  }
> >
> > +static int flexcan_enter_low_power_ack(struct flexcan_priv *priv)
> 
> nitpick: please call this flexcan_low_power_enter_ack()
> > +{
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> > +
> > +	while (timeout-- && !(priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > +		udelay(10);
> > +
> > +	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int flexcan_exit_low_power_ack(struct flexcan_priv *priv)
> 
> ...and this one flexcan_low_power_exit_ack()
> 
> > +{
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> > +
> > +	while (timeout-- && (priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > +		udelay(10);
> > +
> > +	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> 
> Can you move split the creation of
> flexcan_low_power_enter_ack()/flexcan_low_power_exit_ack() and use in
> flexcan_chip_enable/disable() into one patch and the conversion of
> flexcan_enter_stop_mode()/flexcan_exit_stop_mode() into another.

Of course, this could be more clearer and reasonable. Thanks.

Best Regards,
Joakim Zhang
> Marc
> 
> > +
> >  static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> > enable)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs; @@ -406,7 +434,6 @@
> > static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool
> > enable)  static inline int flexcan_enter_stop_mode(struct flexcan_priv
> > *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int ackval;
> >  	u32 reg_mcr;
> >
> >  	reg_mcr = priv->read(&regs->mcr);
> > @@ -418,35 +445,24 @@ static inline int flexcan_enter_stop_mode(struct
> flexcan_priv *priv)
> >  			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
> >
> >  	/* get stop acknowledgment */
> > -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> > -				     ackval, ackval & (1 << priv->stm.ack_bit),
> > -				     0, FLEXCAN_TIMEOUT_US))
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > +	return flexcan_enter_low_power_ack(priv);
> >  }
> >
> >  static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
> > {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int ackval;
> >  	u32 reg_mcr;
> >
> >  	/* remove stop request */
> >  	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
> >  			   1 << priv->stm.req_bit, 0);
> >
> > -	/* get stop acknowledgment */
> > -	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
> > -				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
> > -				     0, FLEXCAN_TIMEOUT_US))
> > -		return -ETIMEDOUT;
> > -
> >  	reg_mcr = priv->read(&regs->mcr);
> >  	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
> >  	priv->write(reg_mcr, &regs->mcr);
> >
> > -	return 0;
> > +	/* get stop acknowledgment */
> > +	return flexcan_exit_low_power_ack(priv);
> >  }
> >
> >  static void flexcan_try_exit_stop_mode(struct flexcan_priv *priv) @@
> > -512,39 +528,25 @@ static inline int flexcan_transceiver_disable(const
> > struct flexcan_priv *priv)  static int flexcan_chip_enable(struct
> > flexcan_priv *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> >  	u32 reg;
> >
> >  	reg = priv->read(&regs->mcr);
> >  	reg &= ~FLEXCAN_MCR_MDIS;
> >  	priv->write(reg, &regs->mcr);
> >
> > -	while (timeout-- && (priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > -		udelay(10);
> > -
> > -	if (priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK)
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > +	return flexcan_exit_low_power_ack(priv);
> >  }
> >
> >  static int flexcan_chip_disable(struct flexcan_priv *priv)  {
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > -	unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
> >  	u32 reg;
> >
> >  	reg = priv->read(&regs->mcr);
> >  	reg |= FLEXCAN_MCR_MDIS;
> >  	priv->write(reg, &regs->mcr);
> >
> > -	while (timeout-- && !(priv->read(&regs->mcr) &
> FLEXCAN_MCR_LPM_ACK))
> > -		udelay(10);
> > -
> > -	if (!(priv->read(&regs->mcr) & FLEXCAN_MCR_LPM_ACK))
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > +	return flexcan_enter_low_power_ack(priv);
> >  }
> >
> >  static int flexcan_chip_freeze(struct flexcan_priv *priv)
> >
> 
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |