pinctrl: sh-pfc: r8a77970: remove SH_PFC_PIN_CFG_DRIVE_STRENGTH flag

Message ID 20180702080544.22665-1-niklas.soderlund+renesas@ragnatech.se
State New
Headers show
Series
  • pinctrl: sh-pfc: r8a77970: remove SH_PFC_PIN_CFG_DRIVE_STRENGTH flag
Related show

Commit Message

Niklas Söderlund July 2, 2018, 8:05 a.m.
The datasheet do not document any registers to control drive strength,
and no drive strength registers are for this reason described for this
SoC. The flag indicating that drive strength can be controlled are
however set for some pins in the driver.

This leads to a NULL pointer dereference when the sh-pfc core tries to
access the struct describing the drive strength registers, for example
when reading the sysfs file pinconf-pins.

Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/pinctrl/sh-pfc/pfc-r8a77970.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Geert Uytterhoeven July 2, 2018, 1:03 p.m. | #1
CC Sergei,

Thanks for your patch!

On Mon, Jul 2, 2018 at 10:06 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The datasheet do not document any registers to control drive strength,

does

> and no drive strength registers are for this reason described for this
> SoC. The flag indicating that drive strength can be controlled are

flags

> however set for some pins in the driver.
>
> This leads to a NULL pointer dereference when the sh-pfc core tries to
> access the struct describing the drive strength registers, for example
> when reading the sysfs file pinconf-pins.
>
> Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I'll wait a bit for Sergei's response.
Perhaps his version of the datasheet does document drive strength registers?

> --- a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
> @@ -21,15 +21,13 @@
>  #include "core.h"
>  #include "sh_pfc.h"
>
> -#define CFG_FLAGS SH_PFC_PIN_CFG_DRIVE_STRENGTH
> -
>  #define CPU_ALL_PORT(fn, sfx)                                          \
> -       PORT_GP_CFG_22(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> -       PORT_GP_CFG_28(1, fn, sfx, CFG_FLAGS),                          \
> -       PORT_GP_CFG_17(2, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> -       PORT_GP_CFG_17(3, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> -       PORT_GP_CFG_6(4,  fn, sfx, CFG_FLAGS),                          \
> -       PORT_GP_CFG_15(5, fn, sfx, CFG_FLAGS)
> +       PORT_GP_CFG_22(0, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> +       PORT_GP_28(1, fn, sfx),                                         \
> +       PORT_GP_CFG_17(2, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> +       PORT_GP_CFG_17(3, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> +       PORT_GP_6(4,  fn, sfx),                                         \
> +       PORT_GP_15(5, fn, sfx)
>  /*
>   * F_() : just information
>   * FM() : macro for FN_xxx / xxx_MARK

Gr{oetje,eeting}s,

                        Geert
Simon Horman July 2, 2018, 2:12 p.m. | #2
On Mon, Jul 02, 2018 at 10:05:44AM +0200, Niklas Söderlund wrote:
> The datasheet do not document any registers to control drive strength,
> and no drive strength registers are for this reason described for this
> SoC. The flag indicating that drive strength can be controlled are
> however set for some pins in the driver.
> 
> This leads to a NULL pointer dereference when the sh-pfc core tries to
> access the struct describing the drive strength registers, for example
> when reading the sysfs file pinconf-pins.
> 
> Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov July 2, 2018, 3:35 p.m. | #3
Hello!

On 7/2/2018 4:03 PM, Geert Uytterhoeven wrote:

> CC Sergei,
> 
> Thanks for your patch!
> 
> On Mon, Jul 2, 2018 at 10:06 AM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
>> The datasheet do not document any registers to control drive strength,
> 
> does
> 
>> and no drive strength registers are for this reason described for this
>> SoC. The flag indicating that drive strength can be controlled are
> 
> flags
> 
>> however set for some pins in the driver.
>>
>> This leads to a NULL pointer dereference when the sh-pfc core tries to
>> access the struct describing the drive strength registers, for example
>> when reading the sysfs file pinconf-pins.
>>
>> Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> I'll wait a bit for Sergei's response.
> Perhaps his version of the datasheet does document drive strength registers?

    No, I have v1.00 which doesn't tell about drive control on V3M indeed.

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov July 2, 2018, 3:39 p.m. | #4
Hello!

On 7/2/2018 11:05 AM, Niklas Söderlund wrote:

> The datasheet do not document any registers to control drive strength,
> and no drive strength registers are for this reason described for this
> SoC. The flag indicating that drive strength can be controlled are
> however set for some pins in the driver.
> 
> This leads to a NULL pointer dereference when the sh-pfc core tries to
> access the struct describing the drive strength registers, for example
> when reading the sysfs file pinconf-pins.
> 
> Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.

    Sorry about overlooking this!

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Niklas Söderlund July 2, 2018, 4:15 p.m. | #5
Hi Geert,

Thanks for your feedback.

On 2018-07-02 15:03:02 +0200, Geert Uytterhoeven wrote:
> CC Sergei,
> 
> Thanks for your patch!
> 
> On Mon, Jul 2, 2018 at 10:06 AM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > The datasheet do not document any registers to control drive strength,
> 
> does
> 
> > and no drive strength registers are for this reason described for this
> > SoC. The flag indicating that drive strength can be controlled are
> 
> flags
> 
> > however set for some pins in the driver.
> >
> > This leads to a NULL pointer dereference when the sh-pfc core tries to
> > access the struct describing the drive strength registers, for example
> > when reading the sysfs file pinconf-pins.
> >
> > Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> I'll wait a bit for Sergei's response.
> Perhaps his version of the datasheet does document drive strength registers?

Do you wish for a v2 addressing the spelling above or can you fix that 
when applying?

> 
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
> > @@ -21,15 +21,13 @@
> >  #include "core.h"
> >  #include "sh_pfc.h"
> >
> > -#define CFG_FLAGS SH_PFC_PIN_CFG_DRIVE_STRENGTH
> > -
> >  #define CPU_ALL_PORT(fn, sfx)                                          \
> > -       PORT_GP_CFG_22(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> > -       PORT_GP_CFG_28(1, fn, sfx, CFG_FLAGS),                          \
> > -       PORT_GP_CFG_17(2, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> > -       PORT_GP_CFG_17(3, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> > -       PORT_GP_CFG_6(4,  fn, sfx, CFG_FLAGS),                          \
> > -       PORT_GP_CFG_15(5, fn, sfx, CFG_FLAGS)
> > +       PORT_GP_CFG_22(0, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> > +       PORT_GP_28(1, fn, sfx),                                         \
> > +       PORT_GP_CFG_17(2, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> > +       PORT_GP_CFG_17(3, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> > +       PORT_GP_6(4,  fn, sfx),                                         \
> > +       PORT_GP_15(5, fn, sfx)
> >  /*
> >   * F_() : just information
> >   * FM() : macro for FN_xxx / xxx_MARK
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Geert Uytterhoeven July 3, 2018, 8:14 a.m. | #6
Hi Niklas,

CC LinusW

On Mon, Jul 2, 2018 at 6:15 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2018-07-02 15:03:02 +0200, Geert Uytterhoeven wrote:
> > CC Sergei,
> >
> > Thanks for your patch!
> >
> > On Mon, Jul 2, 2018 at 10:06 AM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > The datasheet do not document any registers to control drive strength,
> >
> > does
> >
> > > and no drive strength registers are for this reason described for this
> > > SoC. The flag indicating that drive strength can be controlled are
> >
> > flags
> >
> > > however set for some pins in the driver.
> > >
> > > This leads to a NULL pointer dereference when the sh-pfc core tries to
> > > access the struct describing the drive strength registers, for example
> > > when reading the sysfs file pinconf-pins.
> > >
> > > Fix this by removing the SH_PFC_PIN_CFG_DRIVE_STRENGTH from all pins.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll wait a bit for Sergei's response.
> > Perhaps his version of the datasheet does document drive strength registers?
>
> Do you wish for a v2 addressing the spelling above or can you fix that
> when applying?

Given this fixes a crash, I think this is a fix for v4.18.
So please send a v2 straight to LinusW, with

    Fixes: b92ac66a1819602b ("pinctrl: sh-pfc: Add R8A77970 PFC support")

Unfortunately the code has changed since the issue was introduced, so please
send an applicable fix for v4.16/v4.17 to stable after the fix has entered
upstream.

Thanks!

> > > --- a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
> > > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
> > > @@ -21,15 +21,13 @@
> > >  #include "core.h"
> > >  #include "sh_pfc.h"
> > >
> > > -#define CFG_FLAGS SH_PFC_PIN_CFG_DRIVE_STRENGTH
> > > -
> > >  #define CPU_ALL_PORT(fn, sfx)                                          \
> > > -       PORT_GP_CFG_22(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> > > -       PORT_GP_CFG_28(1, fn, sfx, CFG_FLAGS),                          \
> > > -       PORT_GP_CFG_17(2, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> > > -       PORT_GP_CFG_17(3, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
> > > -       PORT_GP_CFG_6(4,  fn, sfx, CFG_FLAGS),                          \
> > > -       PORT_GP_CFG_15(5, fn, sfx, CFG_FLAGS)
> > > +       PORT_GP_CFG_22(0, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> > > +       PORT_GP_28(1, fn, sfx),                                         \
> > > +       PORT_GP_CFG_17(2, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> > > +       PORT_GP_CFG_17(3, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),          \
> > > +       PORT_GP_6(4,  fn, sfx),                                         \
> > > +       PORT_GP_15(5, fn, sfx)
> > >  /*
> > >   * F_() : just information
> > >   * FM() : macro for FN_xxx / xxx_MARK

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
index b02caf31671186d9..eeb58b3bbc9a0cef 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
@@ -21,15 +21,13 @@ 
 #include "core.h"
 #include "sh_pfc.h"
 
-#define CFG_FLAGS SH_PFC_PIN_CFG_DRIVE_STRENGTH
-
 #define CPU_ALL_PORT(fn, sfx)						\
-	PORT_GP_CFG_22(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
-	PORT_GP_CFG_28(1, fn, sfx, CFG_FLAGS),				\
-	PORT_GP_CFG_17(2, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
-	PORT_GP_CFG_17(3, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE), \
-	PORT_GP_CFG_6(4,  fn, sfx, CFG_FLAGS),				\
-	PORT_GP_CFG_15(5, fn, sfx, CFG_FLAGS)
+	PORT_GP_CFG_22(0, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),		\
+	PORT_GP_28(1, fn, sfx),						\
+	PORT_GP_CFG_17(2, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),		\
+	PORT_GP_CFG_17(3, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),		\
+	PORT_GP_6(4,  fn, sfx),						\
+	PORT_GP_15(5, fn, sfx)
 /*
  * F_() : just information
  * FM() : macro for FN_xxx / xxx_MARK