Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K
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.
Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K
On 15/11/2019 05.41, Timur Tabi wrote:
> On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
> 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
Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K
On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
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.
[PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K
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
Signed-off-by: Rasmus Villemoes
---
drivers/net/wan/fsl_ucc_hdlc.c | 5 +
1 file changed, 5 insertions(+)
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, >ucc_pram->riptr);
--
2.23.0
