| Message ID | 20191108130123.6839-46-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | QUICC Engine support on ARM and ARM64 | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (85c5b0984ebb104ec7a0a853ec1e63c19f500313) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c > index 8d13586bb774..f029eaa7cfc0 100644 > --- a/drivers/net/wan/fsl_ucc_hdlc.c > +++ b/drivers/net/wan/fsl_ucc_hdlc.c > @@ -245,6 +245,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv) > ret = -ENOMEM; > goto free_riptr; > } > + if (riptr != (u16)riptr || tiptr != (u16)tiptr) { "riptr/tiptr > U16_MAX" is clearer.
On 15/11/2019 05.41, Timur Tabi wrote: > On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: > >> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c >> index 8d13586bb774..f029eaa7cfc0 100644 >> --- a/drivers/net/wan/fsl_ucc_hdlc.c >> +++ b/drivers/net/wan/fsl_ucc_hdlc.c >> @@ -245,6 +245,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv) >> ret = -ENOMEM; >> goto free_riptr; >> } >> + if (riptr != (u16)riptr || tiptr != (u16)tiptr) { > > "riptr/tiptr > U16_MAX" is clearer. > I can change it, sure, but it's a matter of taste. To me the above asks "does the value change when it is truncated to a u16" which makes perfect sense when the value is next used with iowrite16be(). Using a comparison to U16_MAX takes more brain cycles for me, because I have to think whether it should be > or >=, and are there some signedness/integer promotion business interfering with that test. Rasmus
On 11/15/19 1:44 AM, Rasmus Villemoes wrote: > I can change it, sure, but it's a matter of taste. To me the above asks > "does the value change when it is truncated to a u16" which makes > perfect sense when the value is next used with iowrite16be(). Using a > comparison to U16_MAX takes more brain cycles for me, because I have to > think whether it should be > or >=, and are there some > signedness/integer promotion business interfering with that test. Ok.
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c index 8d13586bb774..f029eaa7cfc0 100644 --- a/drivers/net/wan/fsl_ucc_hdlc.c +++ b/drivers/net/wan/fsl_ucc_hdlc.c @@ -245,6 +245,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv) ret = -ENOMEM; goto free_riptr; } + if (riptr != (u16)riptr || tiptr != (u16)tiptr) { + dev_err(priv->dev, "MURAM allocation out of addressable range\n"); + ret = -ENOMEM; + goto free_tiptr; + } /* Set RIPTR, TIPTR */ iowrite16be(riptr, &priv->ucc_pram->riptr);
Qiang Zhao points out that these offsets get written to 16-bit registers, and there are some QE platforms with more than 64K muram. So it is possible that qe_muram_alloc() gives us an allocation that can't actually be used by the hardware, so detect and reject that. Reported-by: Qiang Zhao <qiang.zhao@nxp.com> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- drivers/net/wan/fsl_ucc_hdlc.c | 5 +++++ 1 file changed, 5 insertions(+)