[PATCH] platform/x86: intel-hid: Support Lenovo ThinkPad X1 Tablet Gen 2
Like a few other system the Lenovo ThinkPad X1 Tablet Gen 2 miss the
HEBC method, which prevent the power button from working. Add a quirk
to enable the button array on this system family and fix the power
button.
Signed-off-by: Alban Bedel
---
drivers/platform/x86/intel-hid.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index 2f5b8d09143e..57cc92891a57 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -90,6 +90,13 @@ static const struct dmi_system_id button_array_table[] = {
DMI_MATCH(DMI_PRODUCT_NAME, "HP Spectre x2 Detachable"),
},
},
+ {
+ .ident = "Lenovo ThinkPad X1 Tablet Gen 2",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad X1 Tablet Gen
2"),
+ },
+ },
{ }
};
--
2.27.0
Re: [PATCH v2] gpio: pca953x: add support for open drain pins on PCAL6524
On Wed, 2021-02-17 at 16:19 +0200, Andy Shevchenko wrote: > On Wed, Feb 17, 2021 at 3:11 PM Bedel, Alban > wrote: > > On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote: > > > On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban < > > > alban.be...@aerq.com> > > > wrote: > > > > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > > > > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel < > > > > > alban.be...@aerq.com > > > > > wrote: > > ... > > > > > > > +#define PCAL65xx_REGS BIT(10) > > > > > > > > > > Can we have it as a _TYPE, please? > > > > > > > > Let's please take a closer look at these macros and what they > > > > mean. > > > > Currently we have 3 possible set of functions that are > > > > indicated by > > > > setting bits in driver_data using the PCA_xxx macros: > > > > > > > > - Basic register only: 0 > > > > - With interrupt support: PCA_INT > > > > - With latching interrupt regs: PCA_INT | PCA_PCAL = > > > > PCA_LATCH_INT > > > > > > > > This patch then add a forth case: > > > > > > > > - With pin config regs: PCA_INT | PCA_PCAL | > > > > $MACRO_WE_ARE_DICUSSING > > > > > > > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which > > > > indicate > > > > the need for a different regmap config and register layout. > > > > > > Exactly, and you have a different register layout (coincidentally > > > overlaps with the original PCA953x). > > > > We have 2 layout for the base registers, the "mixed up registers" > > of > > the PCA957x and the "standard" of the PCA953x. Then we have the > > PCAL chips which extend the base PCA953x registers with further > > registers for better interrupt handling. These are not treated as a > > new > > type in the current code, but as an extra feature on top of the > > PCA953x. > > Yes, because they are about interrupts AFAICS. This distinction seems arbitrary, each more advanced version of the chip just has more features along with a new register block. > > The PCAL65xx we are talking about add a further register > > block, so following the existing concept its not a new layout. > > Yes, with one important detail, i.e. it extends the "mixed up" > registers, it's not a separate "feature" in this sense. The separate > "feature" can be, for example, PWM registers. I admit that this most > of the angle of preference how to draw a line between the features. > > I prefer to see it as a type because of two things (in the current > code): > - OF_9*() macros take only two arguments, second of which is > Interrupt related > - PCA_INT group of bits is about Interrupt only No, the register set indicated by PCA_PCAL also allow setting pull up/down which is supported by this driver. Furthermore the extra registers of the PCAL65XX also allow configuring edge triggered mode for interrupts. I really don't see why there should be 2 class of features, that only make the code more complex. > Your proposal will disrupt the code (more invasive). I tried to implement what you like to see: 1 file changed, 105 insertions(+), 20 deletions(-) vs my proposal: 1 file changed, 65 insertions(+), 3 deletions(-) Alban signature.asc Description: This is a digitally signed message part
Re: [PATCH v2] gpio: pca953x: add support for open drain pins on PCAL6524
On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote:
> On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban
> wrote:
> > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote:
> > > Hint: don't forget to include reviewers from previous version
> >
> > I added you to the CC list for the new patch, did I miss someone
> > else?
>
> Then we are fine, thanks!
>
> > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel > > >
> > > wrote:
> > > > From a quick glance at various datasheets the PCAL6524 and the
> > > > PCAL6534 seems to be the only chips in this family that support
> > > > setting the drive mode of single pins. Other chips either don't
> > > > support it at all, or can only set the drive mode of whole
> > > > banks,
> > > > which doesn't map to the GPIO API.
> > > >
> > > > Add a new flag, PCAL65xx_REGS, to mark chips that have the
> > > > extra
> > > > registers needed for this feature. Then mark the needed
> > > > register
> > > > banks
> > > > as readable and writable, here we don't set OUT_CONF as
> > > > writable,
> > > > although it is, as we only need to read it. Finally add a
> > > > function
> > > > that configures the OUT_INDCONF register when the GPIO API sets
> > > > the
> > > > drive mode of the pins.
>
> Before continuing on this, have you considered to split this
> particular chip to a real pin controller and use the existing driver
> only for GPIO part of the functionality?
No, this driver already use the ->set_config() mechanism so adding
another property is trivial. On the other hand having a pinctrl driver
would be a massive undertaking as the pinctrl API is quiet complex
iirc. Furthermore, unless I'm missing something, that would not allow a
consumer to request an open drain GPIO which is what I'm after.
> > > > +#define PCAL65xx_REGS BIT(10)
> > >
> > > Can we have it as a _TYPE, please?
> >
> > Let's please take a closer look at these macros and what they mean.
> > Currently we have 3 possible set of functions that are indicated by
> > setting bits in driver_data using the PCA_xxx macros:
> >
> > - Basic register only: 0
> > - With interrupt support: PCA_INT
> > - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT
> >
> > This patch then add a forth case:
> >
> > - With pin config regs: PCA_INT | PCA_PCAL |
> > $MACRO_WE_ARE_DICUSSING
> >
> > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which
> > indicate
> > the need for a different regmap config and register layout.
>
> Exactly, and you have a different register layout (coincidentally
> overlaps with the original PCA953x).
We have 2 layout for the base registers, the "mixed up registers" of
the PCA957x and the "standard" of the PCA953x. Then we have the
PCAL chips which extend the base PCA953x registers with further
registers for better interrupt handling. These are not treated as a new
type in the current code, but as an extra feature on top of the
PCA953x. The PCAL65xx we are talking about add a further register
block, so following the existing concept its not a new layout.
> > These are
> > accessed using the PCA_CHIP_TYPE() and are used as an integer
> > value,
> > not as bit-field like the above ones. If we had a struct instead of
> > a
> > packed integer that would be a different field.
>
> How come? It's a bitmask.
The definitions use BIT() but all evaluations of PCA_CHIP_TYPE() use
the equality operator.
> > I'll change it to PCAL65xx_TYPE if you insist, but that seems very
> > wrong to me to use the same naming convention for macros meant for
> > different fields.
>
> To me it's the opposite. The 6524 defines a new type (which has some
> registers others don't have). We even have already definitions of
> those special registers. I think TYPE is a better approach here.
Let's look at pca953x_writeable_register() which I think clearly show
how chips variants are currently handled. This is just one example but
the rest of the code follows the same concept.
static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
{
struct pca953x_chip *chip = dev_get_drvdata(dev);
u32 bank;
if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
PCA953x_BANK_CONFIG;
} else {
bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
PCA957x_BANK_CONFIG | PCA
Re: [PATCH v2] gpio: pca953x: add support for open drain pins on PCAL6524
On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > Hint: don't forget to include reviewers from previous version I added you to the CC list for the new patch, did I miss someone else? > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel > wrote: > > From a quick glance at various datasheets the PCAL6524 and the > > PCAL6534 seems to be the only chips in this family that support > > setting the drive mode of single pins. Other chips either don't > > support it at all, or can only set the drive mode of whole banks, > > which doesn't map to the GPIO API. > > > > Add a new flag, PCAL65xx_REGS, to mark chips that have the extra > > registers needed for this feature. Then mark the needed register > > banks > > as readable and writable, here we don't set OUT_CONF as writable, > > although it is, as we only need to read it. Finally add a function > > that configures the OUT_INDCONF register when the GPIO API sets the > > drive mode of the pins. > > ... > > > +#define PCAL65xx_REGS BIT(10) > > Can we have it as a _TYPE, please? Let's please take a closer look at these macros and what they mean. Currently we have 3 possible set of functions that are indicated by setting bits in driver_data using the PCA_xxx macros: - Basic register only: 0 - With interrupt support: PCA_INT - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT This patch then add a forth case: - With pin config regs: PCA_INT | PCA_PCAL | $MACRO_WE_ARE_DICUSSING Then there is the PCA953X_TYPE and PCA957X_TYPE macros which indicate the need for a different regmap config and register layout. These are accessed using the PCA_CHIP_TYPE() and are used as an integer value, not as bit-field like the above ones. If we had a struct instead of a packed integer that would be a different field. I'll change it to PCAL65xx_TYPE if you insist, but that seems very wrong to me to use the same naming convention for macros meant for different fields. > > > +#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12) > > IND is a bit ambiguous based on the description below. > After you elaborate, I probably can propose better naming. It's derived from the register name in the datasheet which is "Individual pin output port X configuration register". > > + * - PCAL65xx with individual pin configuration > > + * Individual pin output config0x40 + 12 * bank_size RW > > Not sure I understand what "individual" means here (no, I haven't > looked into the datasheet). "individual" mean that each pin can be configured individually as opposed to other chips that only allow to configure a whole bank of pins. > > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN) > > + val = mask; > > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL) > > + val = 0; > > + else > > + return -EINVAL; > > Switch-case will look more naturally here (despite being longer in > terms of LOCs). Ok. > > +exit: > > exit_unlock: Will do. > > + mutex_unlock(>i2c_lock); > > + return ret; > > ... > > > +#define OF_L65XX(__nrgpio) OF_953X(__nrgpio, PCA_LATCH_INT | > > PCAL65xx_REGS) > > When you change to the type, it will go accordingly. Don't add > LATCH_INT to the macro. As explained above all chips with these registers also have the registers indicated by PCA_LATCH_INT. Alban signature.asc Description: This is a digitally signed message part
[PATCH v2] gpio: pca953x: add support for open drain pins on PCAL6524
>From a quick glance at various datasheets the PCAL6524 and the
PCAL6534 seems to be the only chips in this family that support
setting the drive mode of single pins. Other chips either don't
support it at all, or can only set the drive mode of whole banks,
which doesn't map to the GPIO API.
Add a new flag, PCAL65xx_REGS, to mark chips that have the extra
registers needed for this feature. Then mark the needed register banks
as readable and writable, here we don't set OUT_CONF as writable,
although it is, as we only need to read it. Finally add a function
that configures the OUT_INDCONF register when the GPIO API sets the
drive mode of the pins.
Signed-off-by: Alban Bedel
---
v2: - Rename the feature flag to PCAL65xx_REGS as there is at least
the PCAL6534 which also have this feature.
- Fixed the typos in the log message.
- Fixed the code style issues.
---
drivers/gpio/gpio-pca953x.c | 64 +++--
1 file changed, 61 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 825b362eb4b7..c5c216ddfe18 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -64,6 +64,7 @@
#define PCA_INTBIT(8)
#define PCA_PCAL BIT(9)
#define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
+#define PCAL65xx_REGS BIT(10)
#define PCA953X_TYPE BIT(12)
#define PCA957X_TYPE BIT(13)
#define PCA_TYPE_MASK GENMASK(15, 12)
@@ -88,7 +89,7 @@ static const struct i2c_device_id pca953x_id[] = {
{ "pca9698", 40 | PCA953X_TYPE, },
{ "pcal6416", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
- { "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT, },
+ { "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT | PCAL65xx_REGS, },
{ "pcal9535", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal9554b", 8 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal9555a", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
@@ -265,6 +266,9 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
#define PCAL9xxx_BANK_PULL_SEL BIT(8 + 4)
#define PCAL9xxx_BANK_IRQ_MASK BIT(8 + 5)
#define PCAL9xxx_BANK_IRQ_STAT BIT(8 + 6)
+#define PCAL9xxx_BANK_OUT_CONF BIT(8 + 7)
+
+#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12)
/*
* We care about the following registers:
@@ -288,6 +292,10 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
* Pull-up/pull-down select reg0x40 + 4 * bank_sizeRW
* Interrupt mask register 0x40 + 5 * bank_sizeRW
* Interrupt status register 0x40 + 6 * bank_sizeR
+ * Output port configuration 0x40 + 7 * bank_sizeR
+ *
+ * - PCAL65xx with individual pin configuration
+ * Individual pin output config0x40 + 12 * bank_size RW
*
* - Registers with bit 0x80 set, the AI bit
* The bit is cleared and the registers fall into one of the
@@ -336,9 +344,12 @@ static bool pca953x_readable_register(struct device *dev,
unsigned int reg)
if (chip->driver_data & PCA_PCAL) {
bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK |
- PCAL9xxx_BANK_IRQ_STAT;
+ PCAL9xxx_BANK_IRQ_STAT | PCAL9xxx_BANK_OUT_CONF;
}
+ if (chip->driver_data & PCAL65xx_REGS)
+ bank |= PCAL65xx_BANK_INDOUT_CONF;
+
return pca953x_check_register(chip, reg, bank);
}
@@ -359,6 +370,9 @@ static bool pca953x_writeable_register(struct device *dev,
unsigned int reg)
bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK;
+ if (chip->driver_data & PCAL65xx_REGS)
+ bank |= PCAL65xx_BANK_INDOUT_CONF;
+
return pca953x_check_register(chip, reg, bank);
}
@@ -618,6 +632,46 @@ static int pca953x_gpio_set_pull_up_down(struct
pca953x_chip *chip,
return ret;
}
+static int pcal6524_gpio_set_drive_mode(struct pca953x_chip *chip,
+ unsigned int offset,
+ unsigned long config)
+{
+ u8 out_indconf_reg = pca953x_recalc_addr(chip, PCAL6524_OUT_INDCONF,
+offset);
+ u8 out_conf_reg = pca953x_recalc_addr(chip, PCAL953X_OUT_CONF, 0);
+ u8 mask = BIT(offset % BANK_SZ);
+ unsigned int out_conf;
+ int ret;
+ u8 val;
+
+ /* configuration requires the PCAL65xx extended registers */
+ if (!(chip->driver_data & PCAL65xx_REGS))
+ return -ENOTSUPP;
+
+ if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
+ val = mask;
+ else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
+ val = 0;
+ else
+
Re: [PATCH] gpio: pca953x: add support for open drain pins on PCAL6524
On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote:
> On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel
> wrote:
>
> > From a quick glance at various datasheet the PCAL6524 seems to be
> > the
> > only chip in this familly that support setting the drive mode of
> > single pins. Other chips either don't support it at all, or can
> > only
> > set the drive mode of whole banks, which doesn't map to the GPIO
> > API.
> >
> > Add a new flag, PCAL6524, to mark chips that have the extra
> > registers
> > needed for this feature. Then mark the needed register banks as
> > readable and writable, here we don't set OUT_CONF as writable,
> > although it is, as we only need to read it. Finally add a function
> > that configure the OUT_INDCONF register when the GPIO API set the
> > drive mode of the pins.
> >
> > Signed-off-by: Alban Bedel
>
> Thats's nice!
>
> > + * Output port configuration 0x40 + 7 * bank_sizeR
> > + *
> > + * - PCAL6524 with individual pin configuration
> > + * Individual pin output config0x40 + 12 * bank_size RW
>
> So this will become 0x70? It's a bit hard for me this weird
> register layout...
Correct.
> > +static int pcal6524_gpio_set_drive_mode(struct pca953x_chip *chip,
> > + unsigned int offset,
> > + unsigned long config)
> > +{
> > + u8 out_conf_reg = pca953x_recalc_addr(
> > + chip, PCAL953X_OUT_CONF, 0);
> > + u8 out_indconf_reg = pca953x_recalc_addr(
> > + chip, PCAL6524_OUT_INDCONF, offset);
> > + u8 mask = BIT(offset % BANK_SZ), val;
>
> Split to two variable declarations please, this is hard to read.
Will do.
> > + unsigned int out_conf;
> > + int ret;
>
> So we set mask to the bit index for the line we want to affect.
>
> > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
> > + val = mask;
> > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
> > + val = 0;
> > + else
> > + return -EINVAL;
>
> And this makes sense, we set it to 1 to enable open drain.
>
> > + /* Invert the value if ODENn is set */
> > + ret = regmap_read(chip->regmap, out_conf_reg, _conf);
> > + if (ret)
> > + goto exit;
> > + if (out_conf & BIT(offset / BANK_SZ))
>
> I suppose this could be written if (out_conf & mask)?
No, the mask in ODENn is per bank (group of 8 pins) and not per pin.
> > + val ^= mask;
>
> Invert? Why?
>
> The datasheet says:
>
> "If the ODENx bit is set at logic 0 (push-pull), any bit set to
> logic 1
> in the IOCRx register will reverse the output state of that pin
> only
> to open-drain. When ODENx bit is set at logic 1 (open-drain), a
> logic 1 in IOCRx will set that pin to push-pull."
>
> So your logic is accounting for the fact that someone go and set
> one of the bits in ODENx to 1, but aren't they all by default set
> to zero (or should be programmed by the driver to zero)
> so that you can control open drain individually here by simply
> setting the corresponding bit to 1 for open drain and 0 for
> push-pull?
Yes the ODENx bits are 0 by default, but the bootloader might have
changed them for example. Currently the driver doesn't do any reset so
I think it make sense to correctly handle this case as it doesn't bring
that much extra complexity.
Alban
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] gpio: pca953x: add support for open drain pins on PCAL6524
On Tue, 2021-02-02 at 12:42 +0100, Bartosz Golaszewski wrote: > On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel > wrote: > > From a quick glance at various datasheet the PCAL6524 seems to be > > the > > only chip in this familly that support setting the drive mode of > > single pins. Other chips either don't support it at all, or can > > only > > set the drive mode of whole banks, which doesn't map to the GPIO > > API. > > > > Add a new flag, PCAL6524, to mark chips that have the extra > > registers > > needed for this feature. Then mark the needed register banks as > > readable and writable, here we don't set OUT_CONF as writable, > > although it is, as we only need to read it. Finally add a function > > that configure the OUT_INDCONF register when the GPIO API set the > > drive mode of the pins. > > > > Signed-off-by: Alban Bedel > > --- > > drivers/gpio/gpio-pca953x.c | 64 > > +++-- > > 1 file changed, 61 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio- > > pca953x.c > > index 825b362eb4b7..db0b3dab1490 100644 > > --- a/drivers/gpio/gpio-pca953x.c > > +++ b/drivers/gpio/gpio-pca953x.c > > @@ -64,6 +64,8 @@ > > #define PCA_INTBIT(8) > > #define PCA_PCAL BIT(9) > > #define PCA_LATCH_INT (PCA_PCAL | PCA_INT) > > +#define PCAL6524 BIT(10) > > Maybe call it PCAL6524_TYPE for consistency with the ones below? This is a flag that, like the above ones, indicate the existence of registers, the types found under indicate a different register layout. Misusing the naming convention would probably be confusing. A generic name that don't reference the part type (PCA_???) would perhaps be better but as I couldn't find any other part with these registers I left it at that for now. > > + > > No need for this newline. I'll fix all the style issues you pointed out. Alban signature.asc Description: This is a digitally signed message part
[PATCH] gpio: pca953x: add support for open drain pins on PCAL6524
>From a quick glance at various datasheet the PCAL6524 seems to be the
only chip in this familly that support setting the drive mode of
single pins. Other chips either don't support it at all, or can only
set the drive mode of whole banks, which doesn't map to the GPIO API.
Add a new flag, PCAL6524, to mark chips that have the extra registers
needed for this feature. Then mark the needed register banks as
readable and writable, here we don't set OUT_CONF as writable,
although it is, as we only need to read it. Finally add a function
that configure the OUT_INDCONF register when the GPIO API set the
drive mode of the pins.
Signed-off-by: Alban Bedel
---
drivers/gpio/gpio-pca953x.c | 64 +++--
1 file changed, 61 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 825b362eb4b7..db0b3dab1490 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -64,6 +64,8 @@
#define PCA_INTBIT(8)
#define PCA_PCAL BIT(9)
#define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
+#define PCAL6524 BIT(10)
+
#define PCA953X_TYPE BIT(12)
#define PCA957X_TYPE BIT(13)
#define PCA_TYPE_MASK GENMASK(15, 12)
@@ -88,7 +90,7 @@ static const struct i2c_device_id pca953x_id[] = {
{ "pca9698", 40 | PCA953X_TYPE, },
{ "pcal6416", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
- { "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT, },
+ { "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT | PCAL6524, },
{ "pcal9535", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal9554b", 8 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal9555a", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
@@ -265,6 +267,9 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
#define PCAL9xxx_BANK_PULL_SEL BIT(8 + 4)
#define PCAL9xxx_BANK_IRQ_MASK BIT(8 + 5)
#define PCAL9xxx_BANK_IRQ_STAT BIT(8 + 6)
+#define PCAL9xxx_BANK_OUT_CONF BIT(8 + 7)
+
+#define PCAL6524_BANK_INDOUT_CONF BIT(8 + 12)
/*
* We care about the following registers:
@@ -288,6 +293,10 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
* Pull-up/pull-down select reg0x40 + 4 * bank_sizeRW
* Interrupt mask register 0x40 + 5 * bank_sizeRW
* Interrupt status register 0x40 + 6 * bank_sizeR
+ * Output port configuration 0x40 + 7 * bank_sizeR
+ *
+ * - PCAL6524 with individual pin configuration
+ * Individual pin output config0x40 + 12 * bank_size RW
*
* - Registers with bit 0x80 set, the AI bit
* The bit is cleared and the registers fall into one of the
@@ -336,9 +345,12 @@ static bool pca953x_readable_register(struct device *dev,
unsigned int reg)
if (chip->driver_data & PCA_PCAL) {
bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK |
- PCAL9xxx_BANK_IRQ_STAT;
+ PCAL9xxx_BANK_IRQ_STAT | PCAL9xxx_BANK_OUT_CONF;
}
+ if (chip->driver_data & PCAL6524)
+ bank |= PCAL6524_BANK_INDOUT_CONF;
+
return pca953x_check_register(chip, reg, bank);
}
@@ -359,6 +371,9 @@ static bool pca953x_writeable_register(struct device *dev,
unsigned int reg)
bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK;
+ if (chip->driver_data & PCAL6524)
+ bank |= PCAL6524_BANK_INDOUT_CONF;
+
return pca953x_check_register(chip, reg, bank);
}
@@ -618,6 +633,46 @@ static int pca953x_gpio_set_pull_up_down(struct
pca953x_chip *chip,
return ret;
}
+static int pcal6524_gpio_set_drive_mode(struct pca953x_chip *chip,
+ unsigned int offset,
+ unsigned long config)
+{
+ u8 out_conf_reg = pca953x_recalc_addr(
+ chip, PCAL953X_OUT_CONF, 0);
+ u8 out_indconf_reg = pca953x_recalc_addr(
+ chip, PCAL6524_OUT_INDCONF, offset);
+ u8 mask = BIT(offset % BANK_SZ), val;
+ unsigned int out_conf;
+ int ret;
+
+ /* configuration requires PCAL6524 extended registers */
+ if (!(chip->driver_data & PCAL6524))
+ return -ENOTSUPP;
+
+ if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
+ val = mask;
+ else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
+ val = 0;
+ else
+ return -EINVAL;
+
+ mutex_lock(>i2c_lock);
+
+ /* Invert the value if ODENn is set */
+ ret = regmap_read(chip->regmap, out_conf_reg, _conf);
+ if (ret)
+ goto exit;
+ if (out_conf & BIT(offset / BANK_SZ))
+
[PATCH net v2] net: mscc: ocelot: Fix multicast to the CPU port
Multicast entries in the MAC table use the high bits of the MAC
address to encode the ports that should get the packets. But this port
mask does not work for the CPU port, to receive these packets on the
CPU port the MAC_CPU_COPY flag must be set.
Because of this IPv6 was effectively not working because neighbor
solicitations were never received. This was not apparent before commit
9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb
entries) as the IPv6 entries were broken so all incoming IPv6
multicast was then treated as unknown and flooded on all ports.
To fix this problem rework the ocelot_mact_learn() to set the
MAC_CPU_COPY flag when a multicast entry that target the CPU port is
added. For this we have to read back the ports endcoded in the pseudo
MAC address by the caller. It is not a very nice design but that avoid
changing the callers and should make backporting easier.
Signed-off-by: Alban Bedel
Fixes: 9403c158b872 ("net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet
mdb entries")
---
Changelog:
v2: - Fix inside ocelot_mact_learn() as suggested by Vladimir Oltean
to avoid changing the callers and making backport easier.
- Fixed the Fixes tag to have a 12 digits hash
---
drivers/net/ethernet/mscc/ocelot.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c
b/drivers/net/ethernet/mscc/ocelot.c
index 0b9992bd6626..ff87a0bc089c 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -60,14 +60,27 @@ int ocelot_mact_learn(struct ocelot *ocelot, int port,
const unsigned char mac[ETH_ALEN],
unsigned int vid, enum macaccess_entry_type type)
{
+ u32 cmd = ANA_TABLES_MACACCESS_VALID |
+ ANA_TABLES_MACACCESS_DEST_IDX(port) |
+ ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
+ ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN);
+ unsigned int mc_ports;
+
+ /* Set MAC_CPU_COPY if the CPU port is used by a multicast entry */
+ if (type == ENTRYTYPE_MACv4)
+ mc_ports = (mac[1] << 8) | mac[2];
+ else if (type == ENTRYTYPE_MACv6)
+ mc_ports = (mac[0] << 8) | mac[1];
+ else
+ mc_ports = 0;
+
+ if (mc_ports & BIT(ocelot->num_phys_ports))
+ cmd |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
+
ocelot_mact_select(ocelot, mac, vid);
/* Issue a write command */
- ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
-ANA_TABLES_MACACCESS_DEST_IDX(port) |
-ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
-
ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN),
-ANA_TABLES_MACACCESS);
+ ocelot_write(ocelot, cmd, ANA_TABLES_MACACCESS);
return ocelot_mact_wait_for_completion(ocelot);
}
--
2.25.1
Re: [PATCH] net: mscc: ocelot: Fix multicast to the CPU port
On Mon, 2021-01-18 at 18:55 +, Vladimir Oltean wrote:
> Hi Alban,
>
> On Mon, Jan 18, 2021 at 05:03:17PM +0100, Alban Bedel wrote:
> > Multicast entries in the MAC table use the high bits of the MAC
> > address to encode the ports that should get the packets. But this
> > port
> > mask does not work for the CPU port, to receive these packets on
> > the
> > CPU port the MAC_CPU_COPY flag must be set.
> >
> > Because of this IPv6 was effectively not working because neighbor
> > solicitations were never received. This was not apparent before
> > commit
> > 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet
> > mdb
> > entries) as the IPv6 entries were broken so all incoming IPv6
> > multicast was then treated as unknown and flooded on all ports.
> >
> > To fix this problem add a new `flags` parameter to
> > ocelot_mact_learn()
> > and set MAC_CPU_COPY when the CPU port is in the port set. We still
> > leave the CPU port in the bitfield as it doesn't seems to hurt.
> >
> > Signed-off-by: Alban Bedel
> > Fixes: 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain
> > Ethernet mdb entries)
> > ---
>
> Good catch, it seems that I really did not test that patch with
> multicast traffic received on the CPU (and not only that patch, but
> ever
> since, in fact), shame on me.
>
> What I don't like your patch is how it spills over the entire ocelot
> driver, yet still fails to compile. You missed a bunch of
> ocelot_mact_learn calls from ocelot_net.c (8 of them, in fact).
> I don't know which kernel tree you applied this patch to, but clearly
> not "net"/master:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
My board use felix and I build without CONFIG_MSCC_OCELOT_SWITCH so I
missed these, my bad.
> I would prefer to see a more self-contained bug fix, such as
> potentially
> this one:
>
> -[cut here]-
> diff --git a/drivers/net/ethernet/mscc/ocelot.c
> b/drivers/net/ethernet/mscc/ocelot.c
> index a560d6be2a44..4d7443b123bd 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -56,18 +56,46 @@ static void ocelot_mact_select(struct ocelot
> *ocelot,
>
> }
>
> +static unsigned long
> +ocelot_decode_ports_from_mdb(const unsigned char *addr,
> + enum macaccess_entry_type entry_type)
> +{
> + unsigned long ports = 0;
> +
> + if (entry_type == ENTRYTYPE_MACv4) {
> + ports = addr[2];
> + ports |= addr[1] << 8;
> + } else if (entry_type == ENTRYTYPE_MACv6) {
> + ports = addr[1];
> + ports |= addr[0] << 8;
> + }
> +
> + return ports;
> +}
> +
> int ocelot_mact_learn(struct ocelot *ocelot, int port,
> const unsigned char mac[ETH_ALEN],
> unsigned int vid, enum macaccess_entry_type type)
> {
> + u32 flags = ANA_TABLES_MACACCESS_VALID |
> + ANA_TABLES_MACACCESS_DEST_IDX(port) |
> + ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
> + ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LE
> ARN);
> +
> ocelot_mact_select(ocelot, mac, vid);
>
> + /* Little API trickery to make this function "just work" when
> the CPU
> + * port module is included in the port mask for multicast IP
> entries.
> + */
> + if (type == ENTRYTYPE_MACv4 || type == ENTRYTYPE_MACv6) {
> + unsigned long ports = ocelot_decode_ports_from_mdb(mac,
> type);
> +
> + if (ports & BIT(ocelot->num_phys_ports))
> + flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
> + }
> +
> /* Issue a write command */
> - ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
> - ANA_TABLES_MACACCESS_DEST_IDX(port) |
> - ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
> - ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCE
> SS_CMD_LEARN),
> - ANA_TABLES_MACACCESS);
> + ocelot_write(ocelot, flags, ANA_TABLES_MACACCESS);
>
> return ocelot_mact_wait_for_completion(ocelot);
> }
> -[cut here]-
>
> It has the advantage of actually compiling, plus it should be easier
> to backport because the changes are all in one place.
>
>
> Please make sure to read:
> Documentation/process/submitting-patches.rst
> (this will tell you what is wrong with your Fixes: tag)
> Documentation/networking/netdev-FAQ.rst
> (this will tell you what is wrong with this patch's --subject-prefix,
> and why the patch does not build on the trees it is supposed to be
> applied to):
> https://patchwork.kernel.org/project/netdevbpf/patch/20210118160317.554018-1-alban.be...@aerq.com/
I must say that this condescending tone is a real turn off.
Alban
signature.asc
Description: This is a digitally signed message part
[PATCH] net: mscc: ocelot: Fix multicast to the CPU port
Multicast entries in the MAC table use the high bits of the MAC
address to encode the ports that should get the packets. But this port
mask does not work for the CPU port, to receive these packets on the
CPU port the MAC_CPU_COPY flag must be set.
Because of this IPv6 was effectively not working because neighbor
solicitations were never received. This was not apparent before commit
9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb
entries) as the IPv6 entries were broken so all incoming IPv6
multicast was then treated as unknown and flooded on all ports.
To fix this problem add a new `flags` parameter to ocelot_mact_learn()
and set MAC_CPU_COPY when the CPU port is in the port set. We still
leave the CPU port in the bitfield as it doesn't seems to hurt.
Signed-off-by: Alban Bedel
Fixes: 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb
entries)
---
drivers/net/ethernet/mscc/ocelot.c | 17 -
drivers/net/ethernet/mscc/ocelot.h | 3 ++-
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c
b/drivers/net/ethernet/mscc/ocelot.c
index 0b9992bd6626..c33162dbf075 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -58,12 +58,13 @@ static void ocelot_mact_select(struct ocelot *ocelot,
int ocelot_mact_learn(struct ocelot *ocelot, int port,
const unsigned char mac[ETH_ALEN],
- unsigned int vid, enum macaccess_entry_type type)
+ unsigned int vid, enum macaccess_entry_type type,
+ u32 flags)
{
ocelot_mact_select(ocelot, mac, vid);
/* Issue a write command */
- ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
+ ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID | flags |
ANA_TABLES_MACACCESS_DEST_IDX(port) |
ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN),
@@ -574,7 +575,7 @@ int ocelot_fdb_add(struct ocelot *ocelot, int port,
if (port == ocelot->npi)
pgid = PGID_CPU;
- return ocelot_mact_learn(ocelot, pgid, addr, vid, ENTRYTYPE_LOCKED);
+ return ocelot_mact_learn(ocelot, pgid, addr, vid, ENTRYTYPE_LOCKED, 0);
}
EXPORT_SYMBOL(ocelot_fdb_add);
@@ -1064,6 +1065,7 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
struct ocelot_multicast *mc;
struct ocelot_pgid *pgid;
u16 vid = mdb->vid;
+ u32 flags = 0;
if (port == ocelot->npi)
port = ocelot->num_phys_ports;
@@ -1107,9 +1109,11 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
mc->entry_type != ENTRYTYPE_MACv6)
ocelot_write_rix(ocelot, pgid->ports, ANA_PGID_PGID,
pgid->index);
+ if (mc->ports & BIT(ocelot->num_phys_ports))
+ flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
return ocelot_mact_learn(ocelot, pgid->index, addr, vid,
-mc->entry_type);
+mc->entry_type, flags);
}
EXPORT_SYMBOL(ocelot_port_mdb_add);
@@ -1120,6 +1124,7 @@ int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
struct ocelot_multicast *mc;
struct ocelot_pgid *pgid;
u16 vid = mdb->vid;
+ u32 flags = 0;
if (port == ocelot->npi)
port = ocelot->num_phys_ports;
@@ -1151,9 +1156,11 @@ int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
mc->entry_type != ENTRYTYPE_MACv6)
ocelot_write_rix(ocelot, pgid->ports, ANA_PGID_PGID,
pgid->index);
+ if (mc->ports & BIT(ocelot->num_phys_ports))
+ flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
return ocelot_mact_learn(ocelot, pgid->index, addr, vid,
-mc->entry_type);
+mc->entry_type, flags);
}
EXPORT_SYMBOL(ocelot_port_mdb_del);
diff --git a/drivers/net/ethernet/mscc/ocelot.h
b/drivers/net/ethernet/mscc/ocelot.h
index 291d39d49c4e..63045f1ef0cd 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -106,7 +106,8 @@ int ocelot_port_fdb_do_dump(const unsigned char *addr, u16
vid,
bool is_static, void *data);
int ocelot_mact_learn(struct ocelot *ocelot, int port,
const unsigned char mac[ETH_ALEN],
- unsigned int vid, enum macaccess_entry_type type);
+ unsigned int vid, enum macaccess_entry_type type,
+ u32 flags);
int ocelot_mact_forget(struct ocelot *ocelot,
const unsigned char mac[ETH_ALEN], unsigned int vid);
int ocelot_port_lag_join(struct ocelot *ocelot, int port,
--
2.25.1
SECCOMP_IOCTL_NOTIF_ADDFD race condition
nt would close(seccompFd) and we will be in a situation where both the seccomp-fd and the unix socket are not attached to any process but they reference each other, so they cannot be closed. What do you think? Is there a better solution? Cheers Alban
Re: [PATCH v4 0/2] NFS: Fix interaction between fs_context and user namespaces
clientaddr", "127.0.0.1", 0) = 0 fsconfig(7, FSCONFIG_CMD_CREATE, NULL, NULL, 0) = 0 fsmount(7, FSMOUNT_CLOEXEC, 0) = 6 move_mount(6, "", AT_FDCWD, "/mnt/nfs", MOVE_MOUNT_F_EMPTY_PATH) = 0 +++ exited with 0 +++ ./nfs-mount-in-userns returned 0 last dmesg line about nfs4_create_server [55258.702256] nfs4_create_server: Using creds from non-init userns 459 55 0:40 / /mnt/nfs rw,relatime shared:187 - nfs4 127.0.0.1:/server rw,vers=4.2,rsize=524288,wsize=524288,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=none,addr=127.0.0.1 + : 'Files on the NFS server:' + ls -lani /server/ total 20 1048578 drwxr-xr-x. 500 4096 Nov 10 09:19 . 2 dr-xr-xr-x. 2100 4096 Nov 9 14:25 .. 1048582 drwx--. 200 4096 Nov 10 09:19 dir-0 1048583 drwx--. 2 1000 1000 4096 Nov 10 09:19 dir-1000 1048584 drwx--. 2 3000 3000 4096 Nov 10 09:19 dir-3000 1048579 -rw---. 1000 Nov 10 09:19 file-0 1048580 -rw---. 1 1000 10000 Nov 10 09:19 file-1000 1048581 -rw---. 1 3000 30000 Nov 10 09:19 file-3000 + : 'Files on the NFS mountpoint (from container PoV):' + nsenter -U -m -n -t 4002 sh -c 'ls -lani /mnt/nfs' total 20 1048578 drwxr-xr-x. 5 0 0 4096 Nov 10 09:19 . 786433 drwxr-xr-x. 3 65534 65534 4096 May 16 16:08 .. 1048582 drwx--. 2 0 0 4096 Nov 10 09:19 dir-0 1048583 drwx--. 2 65534 65534 4096 Nov 10 09:19 dir-1000 1048584 drwx--. 2 65534 65534 4096 Nov 10 09:19 dir-3000 1048579 -rw---. 1 0 00 Nov 10 09:19 file-0 1048580 -rw---. 1 65534 655340 Nov 10 09:19 file-1000 1048581 -rw---. 1 65534 655340 Nov 10 09:19 file-3000 + : 'Files on the NFS mountpoint (from host PoV):' + ls -lani /mnt/nfs/ total 20 1048578 drwxr-xr-x. 5 1000 1000 4096 Nov 10 09:19 . 786433 drwxr-xr-x. 3 0 0 4096 May 16 16:08 .. 1048582 drwx--. 2 1000 1000 4096 Nov 10 09:19 dir-0 1048583 drwx--. 2 4294967294 4294967294 4096 Nov 10 09:19 dir-1000 1048584 drwx--. 2 4294967294 4294967294 4096 Nov 10 09:19 dir-3000 1048579 -rw---. 1 1000 10000 Nov 10 09:19 file-0 1048580 -rw---. 1 4294967294 42949672940 Nov 10 09:19 file-1000 1048581 -rw---. 1 4294967294 42949672940 Nov 10 09:19 file-3000 Alban On Mon, 2 Nov 2020 at 18:48, Sargun Dhillon wrote: > > This is effectively a resend, but re-based atop Anna's current tree. I can > add the samples back in an another patchset. > > Right now, it is possible to mount NFS with an non-matching super block > user ns, and NFS sunrpc user ns. This (for the user) results in an awkward > set of interactions if using anything other than auth_null, where the UIDs > being sent to the server are different than the local UIDs being checked. > This can cause "breakage", where if you try to communicate with the NFS > server with any other set of mappings, it breaks. > > This is after the initial v5.10 merge window, so hopefully this patchset > can be reconsidered, and maybe we can make forward progress? I think that > it takes a relatively conservative approach in enabling user namespaces, > and it prevents the case where someone is using auth_gss (for now), as the > mappings are non-trivial. > > Changes since v3: > * Rebase atop Anna's tree > Changes since v2: > * Removed samples > * Split out NFSv2/v3 patchset from NFSv4 patchset > * Added restrictions around use > Changes since v1: > * Added samples > > Sargun Dhillon (2): > NFS: NFSv2/NFSv3: Use cred from fs_context during mount > NFSv4: Refactor NFS to use user namespaces > > fs/nfs/client.c | 10 -- > fs/nfs/nfs4client.c | 27 ++- > fs/nfs/nfs4idmap.c | 2 +- > fs/nfs/nfs4idmap.h | 3 ++- > 4 files changed, 37 insertions(+), 5 deletions(-) > > > base-commit: 8c39076c276be0b31982e44654e2c2357473258a > -- > 2.25.1 >
[PATCH v4 2/3] dt-bindings: hwmon: Add the +vs supply to the lm75 bindings
Some boards might have a regulator that control the +VS supply, add it
to the bindings.
Signed-off-by: Alban Bedel
Acked-by: Rob Herring
---
v2: Removed the unneeded `maxItems` attribute
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index c9a001627945..96eed5cc7841 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -43,6 +43,9 @@ properties:
reg:
maxItems: 1
+ vs-supply:
+description: phandle to the regulator that provides the +VS supply
+
required:
- compatible
- reg
@@ -58,5 +61,6 @@ examples:
sensor@48 {
compatible = "st,stlm75";
reg = <0x48>;
+vs-supply = <>;
};
};
--
2.25.1
[PATCH v4 1/3] dt-bindings: hwmon: Convert lm75 bindings to yaml
In order to automate the verification of DT nodes convert lm75.txt to
lm75.yaml.
Signed-off-by: Alban Bedel
---
v2: Fix the example to pass `make dt_binding_check`
v4: Add the missing additionalProperties: false
---
.../devicetree/bindings/hwmon/lm75.txt| 39
.../devicetree/bindings/hwmon/lm75.yaml | 62 +++
2 files changed, 62 insertions(+), 39 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt
create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.txt
b/Documentation/devicetree/bindings/hwmon/lm75.txt
deleted file mode 100644
index 273616702c51..
--- a/Documentation/devicetree/bindings/hwmon/lm75.txt
+++ /dev/null
@@ -1,39 +0,0 @@
-*LM75 hwmon sensor.
-
-Required properties:
-- compatible: manufacturer and chip name, one of
- "adi,adt75",
- "dallas,ds1775",
- "dallas,ds75",
- "dallas,ds7505",
- "gmt,g751",
- "national,lm75",
- "national,lm75a",
- "national,lm75b",
- "maxim,max6625",
- "maxim,max6626",
- "maxim,max31725",
- "maxim,max31726",
- "maxim,mcp980x",
- "nxp,pct2075",
- "st,stds75",
- "st,stlm75",
- "microchip,tcn75",
- "ti,tmp100",
- "ti,tmp101",
- "ti,tmp105",
- "ti,tmp112",
- "ti,tmp175",
- "ti,tmp275",
- "ti,tmp75",
- "ti,tmp75b",
- "ti,tmp75c",
-
-- reg: I2C bus address of the device
-
-Example:
-
-sensor@48 {
- compatible = "st,stlm75";
- reg = <0x48>;
-};
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
new file mode 100644
index ..c9a001627945
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/lm75.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LM75 hwmon sensor
+
+maintainers:
+ - Jean Delvare
+ - Guenter Roeck
+
+properties:
+ compatible:
+enum:
+ - adi,adt75
+ - dallas,ds1775
+ - dallas,ds75
+ - dallas,ds7505
+ - gmt,g751
+ - national,lm75
+ - national,lm75a
+ - national,lm75b
+ - maxim,max6625
+ - maxim,max6626
+ - maxim,max31725
+ - maxim,max31726
+ - maxim,mcp980x
+ - nxp,pct2075
+ - st,stds75
+ - st,stlm75
+ - microchip,tcn75
+ - ti,tmp100
+ - ti,tmp101
+ - ti,tmp105
+ - ti,tmp112
+ - ti,tmp175
+ - ti,tmp275
+ - ti,tmp75
+ - ti,tmp75b
+ - ti,tmp75c
+
+ reg:
+maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@48 {
+compatible = "st,stlm75";
+reg = <0x48>;
+ };
+};
--
2.25.1
[PATCH v4 0/3] hwmon: (lm75) Add regulator support
Hi everybody, this small series add regulator support to the lm75 driver for boards that don't always power such a sensor. While at it also convert the DT bindings to yaml. v2: - Fixed the DT example while converting to YAML - Removed the unneeded maxItems from the binding documentation - Use dummy regulator insted of explicitly handling missing regulator v3: - Use a devm action to handle disabling the regulator v4: - Added the missing `additionalProperties: false` when converting the binding documentation to yaml Alban Bedel (3): dt-bindings: hwmon: Convert lm75 bindings to yaml dt-bindings: hwmon: Add the +vs supply to the lm75 bindings hwmon: (lm75) Add regulator support .../devicetree/bindings/hwmon/lm75.txt| 39 --- .../devicetree/bindings/hwmon/lm75.yaml | 66 +++ drivers/hwmon/lm75.c | 24 +++ 3 files changed, 90 insertions(+), 39 deletions(-) delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml -- 2.25.1
[PATCH v4 3/3] hwmon: (lm75) Add regulator support
Add regulator support for boards where the sensor first need to be
powered up before it can be used.
Signed-off-by: Alban Bedel
---
v2: Rely on dummy regulators instead of explicitly handling missing
regulator
v3: Use a devm action to handle disabling the regulator
---
drivers/hwmon/lm75.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index ba0be48aeadd..e9c1f55b2706 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "lm75.h"
/*
@@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { 0x48, 0x49,
0x4a, 0x4b, 0x4c,
struct lm75_data {
struct i2c_client *client;
struct regmap *regmap;
+ struct regulator*vs;
u8 orig_conf;
u8 current_conf;
u8 resolution; /* In bits, 9 to 16 */
@@ -534,6 +536,13 @@ static const struct regmap_config lm75_regmap_config = {
.use_single_write = true,
};
+static void lm75_disable_regulator(void *data)
+{
+ struct lm75_data *lm75 = data;
+
+ regulator_disable(lm75->vs);
+}
+
static void lm75_remove(void *data)
{
struct lm75_data *lm75 = data;
@@ -567,6 +576,10 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->client = client;
data->kind = kind;
+ data->vs = devm_regulator_get(dev, "vs");
+ if (IS_ERR(data->vs))
+ return PTR_ERR(data->vs);
+
data->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(data->regmap))
return PTR_ERR(data->regmap);
@@ -581,6 +594,17 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->sample_time = data->params->default_sample_time;
data->resolution = data->params->default_resolution;
+ /* Enable the power */
+ err = regulator_enable(data->vs);
+ if (err) {
+ dev_err(dev, "failed to enable regulator: %d\n", err);
+ return err;
+ }
+
+ err = devm_add_action_or_reset(dev, lm75_disable_regulator, data);
+ if (err)
+ return err;
+
/* Cache original configuration */
status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
if (status < 0) {
--
2.25.1
[PATCH v3 2/3] dt-bindings: hwmon: Add the +vs supply to the lm75 bindings
Some boards might have a regulator that control the +VS supply, add it
to the bindings.
Signed-off-by: Alban Bedel
---
v2: Removed the unneeded `maxItems` attribute
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index 203829c6ba6f..1c1ce4cf1616 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -43,6 +43,9 @@ properties:
reg:
maxItems: 1
+ vs-supply:
+description: phandle to the regulator that provides the +VS supply
+
required:
- compatible
- reg
@@ -56,5 +59,6 @@ examples:
sensor@48 {
compatible = "st,stlm75";
reg = <0x48>;
+vs-supply = <>;
};
};
--
2.25.1
[PATCH v3 3/3] hwmon: (lm75) Add regulator support
Add regulator support for boards where the sensor first need to be
powered up before it can be used.
Signed-off-by: Alban Bedel
---
v2: Rely on dummy regulators instead of explicitly handling missing
regulator
v3: Use a devm action to handle disabling the regulator
---
drivers/hwmon/lm75.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index ba0be48aeadd..e9c1f55b2706 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "lm75.h"
/*
@@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { 0x48, 0x49,
0x4a, 0x4b, 0x4c,
struct lm75_data {
struct i2c_client *client;
struct regmap *regmap;
+ struct regulator*vs;
u8 orig_conf;
u8 current_conf;
u8 resolution; /* In bits, 9 to 16 */
@@ -534,6 +536,13 @@ static const struct regmap_config lm75_regmap_config = {
.use_single_write = true,
};
+static void lm75_disable_regulator(void *data)
+{
+ struct lm75_data *lm75 = data;
+
+ regulator_disable(lm75->vs);
+}
+
static void lm75_remove(void *data)
{
struct lm75_data *lm75 = data;
@@ -567,6 +576,10 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->client = client;
data->kind = kind;
+ data->vs = devm_regulator_get(dev, "vs");
+ if (IS_ERR(data->vs))
+ return PTR_ERR(data->vs);
+
data->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(data->regmap))
return PTR_ERR(data->regmap);
@@ -581,6 +594,17 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->sample_time = data->params->default_sample_time;
data->resolution = data->params->default_resolution;
+ /* Enable the power */
+ err = regulator_enable(data->vs);
+ if (err) {
+ dev_err(dev, "failed to enable regulator: %d\n", err);
+ return err;
+ }
+
+ err = devm_add_action_or_reset(dev, lm75_disable_regulator, data);
+ if (err)
+ return err;
+
/* Cache original configuration */
status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
if (status < 0) {
--
2.25.1
[PATCH v3 1/3] dt-bindings: hwmon: Convert lm75 bindings to yaml
In order to automate the verification of DT nodes convert lm75.txt to
lm75.yaml.
Signed-off-by: Alban Bedel
---
v2: Fix the example to pass `make dt_binding_check`
---
.../devicetree/bindings/hwmon/lm75.txt| 39
.../devicetree/bindings/hwmon/lm75.yaml | 60 +++
2 files changed, 60 insertions(+), 39 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt
create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.txt
b/Documentation/devicetree/bindings/hwmon/lm75.txt
deleted file mode 100644
index 273616702c51..
--- a/Documentation/devicetree/bindings/hwmon/lm75.txt
+++ /dev/null
@@ -1,39 +0,0 @@
-*LM75 hwmon sensor.
-
-Required properties:
-- compatible: manufacturer and chip name, one of
- "adi,adt75",
- "dallas,ds1775",
- "dallas,ds75",
- "dallas,ds7505",
- "gmt,g751",
- "national,lm75",
- "national,lm75a",
- "national,lm75b",
- "maxim,max6625",
- "maxim,max6626",
- "maxim,max31725",
- "maxim,max31726",
- "maxim,mcp980x",
- "nxp,pct2075",
- "st,stds75",
- "st,stlm75",
- "microchip,tcn75",
- "ti,tmp100",
- "ti,tmp101",
- "ti,tmp105",
- "ti,tmp112",
- "ti,tmp175",
- "ti,tmp275",
- "ti,tmp75",
- "ti,tmp75b",
- "ti,tmp75c",
-
-- reg: I2C bus address of the device
-
-Example:
-
-sensor@48 {
- compatible = "st,stlm75";
- reg = <0x48>;
-};
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
new file mode 100644
index ..203829c6ba6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/lm75.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LM75 hwmon sensor
+
+maintainers:
+ - Jean Delvare
+ - Guenter Roeck
+
+properties:
+ compatible:
+enum:
+ - adi,adt75
+ - dallas,ds1775
+ - dallas,ds75
+ - dallas,ds7505
+ - gmt,g751
+ - national,lm75
+ - national,lm75a
+ - national,lm75b
+ - maxim,max6625
+ - maxim,max6626
+ - maxim,max31725
+ - maxim,max31726
+ - maxim,mcp980x
+ - nxp,pct2075
+ - st,stds75
+ - st,stlm75
+ - microchip,tcn75
+ - ti,tmp100
+ - ti,tmp101
+ - ti,tmp105
+ - ti,tmp112
+ - ti,tmp175
+ - ti,tmp275
+ - ti,tmp75
+ - ti,tmp75b
+ - ti,tmp75c
+
+ reg:
+maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@48 {
+compatible = "st,stlm75";
+reg = <0x48>;
+ };
+};
--
2.25.1
[PATCH v3 0/3] hwmon: (lm75) Add regulator support
v2: - Fixed the DT example while converting to YAML - Removed the unneeded maxItems from the binding documentation - Use dummy regulator insted of explicitly handling missing regulator v3: - Use a devm action to handle disabling the regulator Alban Bedel (3): dt-bindings: hwmon: Convert lm75 bindings to yaml dt-bindings: hwmon: Add the +vs supply to the lm75 bindings hwmon: (lm75) Add regulator support .../devicetree/bindings/hwmon/lm75.txt| 39 --- .../devicetree/bindings/hwmon/lm75.yaml | 64 +++ drivers/hwmon/lm75.c | 24 +++ 3 files changed, 88 insertions(+), 39 deletions(-) delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml -- 2.25.1
Re: [PATCH v2 3/3] hwmon: (lm75) Add regulator support
On Mon, 2020-09-28 at 09:39 -0700, Guenter Roeck wrote:
> On Mon, Sep 28, 2020 at 05:39:23PM +0200, Alban Bedel wrote:
> > Add regulator support for boards where the sensor first need to be
> > powered up before it can be used.
> >
> > Signed-off-by: Alban Bedel
> > ---
> > v2: Rely on dummy regulators instead of explicitly handling missing
> > regulator
> > ---
> > drivers/hwmon/lm75.c | 23 +--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > index ba0be48aeadd..e394df648c26 100644
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "lm75.h"
> >
> > /*
> > @@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = {
> > 0x48, 0x49, 0x4a, 0x4b, 0x4c,
> > struct lm75_data {
> > struct i2c_client *client;
> > struct regmap *regmap;
> > + struct regulator*vs;
> > u8 orig_conf;
> > u8 current_conf;
> > u8 resolution; /* In bits, 9 to 16
> > */
> > @@ -540,6 +542,7 @@ static void lm75_remove(void *data)
> > struct i2c_client *client = lm75->client;
> >
> > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75-
> > >orig_conf);
> > + regulator_disable(lm75->vs);
> > }
> >
> > static int
> > @@ -567,6 +570,10 @@ lm75_probe(struct i2c_client *client, const
> > struct i2c_device_id *id)
> > data->client = client;
> > data->kind = kind;
> >
> > + data->vs = devm_regulator_get(dev, "vs");
> > + if (IS_ERR(data->vs))
> > + return PTR_ERR(data->vs);
> > +
> > data->regmap = devm_regmap_init_i2c(client,
> > _regmap_config);
> > if (IS_ERR(data->regmap))
> > return PTR_ERR(data->regmap);
> > @@ -581,11 +588,19 @@ lm75_probe(struct i2c_client *client, const
> > struct i2c_device_id *id)
> > data->sample_time = data->params->default_sample_time;
> > data->resolution = data->params->default_resolution;
> >
> > + /* Enable the power */
> > + err = regulator_enable(data->vs);
> > + if (err) {
> > + dev_err(dev, "failed to enable regulator: %d\n", err);
> > + return err;
> > + }
> > +
> > /* Cache original configuration */
> > status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
> > if (status < 0) {
> > dev_dbg(dev, "Can't read config? %d\n", status);
> > - return status;
> > + err = status;
> > + goto disable_regulator;
>
> The point of using devm_add_action_or_reset() was specifically to avoid having
> to have the cleanup gotos. On top of that, the lm75_remove() function was
> specifically intended to clean up configuration data, not to do anything else.
> While hijacking lm75_remove() to also disable the regulator is technically
> correct, it makes the code more difficult to understand, and it creates a
> potential source for subsequently introduced bugs. Right now I am not inclined
> to accept this code as-is. Please provide arguments for handling the cleanup
> this way instead of using devm_add_action_or_reset().
I wrote it that way to keep the memory usage lower, but I see your
point and will update the patch accordingly.
Alban
signature.asc
Description: This is a digitally signed message part
[PATCH v2 3/3] hwmon: (lm75) Add regulator support
Add regulator support for boards where the sensor first need to be
powered up before it can be used.
Signed-off-by: Alban Bedel
---
v2: Rely on dummy regulators instead of explicitly handling missing
regulator
---
drivers/hwmon/lm75.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index ba0be48aeadd..e394df648c26 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "lm75.h"
/*
@@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { 0x48, 0x49,
0x4a, 0x4b, 0x4c,
struct lm75_data {
struct i2c_client *client;
struct regmap *regmap;
+ struct regulator*vs;
u8 orig_conf;
u8 current_conf;
u8 resolution; /* In bits, 9 to 16 */
@@ -540,6 +542,7 @@ static void lm75_remove(void *data)
struct i2c_client *client = lm75->client;
i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
+ regulator_disable(lm75->vs);
}
static int
@@ -567,6 +570,10 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->client = client;
data->kind = kind;
+ data->vs = devm_regulator_get(dev, "vs");
+ if (IS_ERR(data->vs))
+ return PTR_ERR(data->vs);
+
data->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(data->regmap))
return PTR_ERR(data->regmap);
@@ -581,11 +588,19 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->sample_time = data->params->default_sample_time;
data->resolution = data->params->default_resolution;
+ /* Enable the power */
+ err = regulator_enable(data->vs);
+ if (err) {
+ dev_err(dev, "failed to enable regulator: %d\n", err);
+ return err;
+ }
+
/* Cache original configuration */
status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
if (status < 0) {
dev_dbg(dev, "Can't read config? %d\n", status);
- return status;
+ err = status;
+ goto disable_regulator;
}
data->orig_conf = status;
data->current_conf = status;
@@ -593,7 +608,7 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
err = lm75_write_config(data, data->params->set_mask,
data->params->clr_mask);
if (err)
- return err;
+ goto disable_regulator;
err = devm_add_action_or_reset(dev, lm75_remove, data);
if (err)
@@ -608,6 +623,10 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
return 0;
+
+disable_regulator:
+ regulator_disable(data->vs);
+ return err;
}
static const struct i2c_device_id lm75_ids[] = {
--
2.25.1
[PATCH v2 1/3] dt-bindings: hwmon: Convert lm75 bindings to yaml
In order to automate the verification of DT nodes convert lm75.txt to
lm75.yaml.
Signed-off-by: Alban Bedel
---
v2: Fix the example to pass `make dt_binding_check`
---
.../devicetree/bindings/hwmon/lm75.txt| 39
.../devicetree/bindings/hwmon/lm75.yaml | 60 +++
2 files changed, 60 insertions(+), 39 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt
create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.txt
b/Documentation/devicetree/bindings/hwmon/lm75.txt
deleted file mode 100644
index 273616702c51..
--- a/Documentation/devicetree/bindings/hwmon/lm75.txt
+++ /dev/null
@@ -1,39 +0,0 @@
-*LM75 hwmon sensor.
-
-Required properties:
-- compatible: manufacturer and chip name, one of
- "adi,adt75",
- "dallas,ds1775",
- "dallas,ds75",
- "dallas,ds7505",
- "gmt,g751",
- "national,lm75",
- "national,lm75a",
- "national,lm75b",
- "maxim,max6625",
- "maxim,max6626",
- "maxim,max31725",
- "maxim,max31726",
- "maxim,mcp980x",
- "nxp,pct2075",
- "st,stds75",
- "st,stlm75",
- "microchip,tcn75",
- "ti,tmp100",
- "ti,tmp101",
- "ti,tmp105",
- "ti,tmp112",
- "ti,tmp175",
- "ti,tmp275",
- "ti,tmp75",
- "ti,tmp75b",
- "ti,tmp75c",
-
-- reg: I2C bus address of the device
-
-Example:
-
-sensor@48 {
- compatible = "st,stlm75";
- reg = <0x48>;
-};
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
new file mode 100644
index ..203829c6ba6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/lm75.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LM75 hwmon sensor
+
+maintainers:
+ - Jean Delvare
+ - Guenter Roeck
+
+properties:
+ compatible:
+enum:
+ - adi,adt75
+ - dallas,ds1775
+ - dallas,ds75
+ - dallas,ds7505
+ - gmt,g751
+ - national,lm75
+ - national,lm75a
+ - national,lm75b
+ - maxim,max6625
+ - maxim,max6626
+ - maxim,max31725
+ - maxim,max31726
+ - maxim,mcp980x
+ - nxp,pct2075
+ - st,stds75
+ - st,stlm75
+ - microchip,tcn75
+ - ti,tmp100
+ - ti,tmp101
+ - ti,tmp105
+ - ti,tmp112
+ - ti,tmp175
+ - ti,tmp275
+ - ti,tmp75
+ - ti,tmp75b
+ - ti,tmp75c
+
+ reg:
+maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@48 {
+compatible = "st,stlm75";
+reg = <0x48>;
+ };
+};
--
2.25.1
[PATCH v2 0/3] hwmon: (lm75) Add regulator support
Hi everybody, this small series add regulator support to the lm75 driver for boards that don't always power such a sensor. While at it also convert the DT bindings to yaml. v2: - Fixed the DT example while converting to YAML - Removed the unneeded maxItems from the binding documentation - Use dummy regulator insted of explicitly handling missing regulator Alban Bedel (3): dt-bindings: hwmon: Convert lm75 bindings to yaml dt-bindings: hwmon: Add the +vs supply to the lm75 bindings hwmon: (lm75) Add regulator support .../devicetree/bindings/hwmon/lm75.txt| 39 --- .../devicetree/bindings/hwmon/lm75.yaml | 64 +++ drivers/hwmon/lm75.c | 23 ++- 3 files changed, 85 insertions(+), 41 deletions(-) delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml -- 2.25.1
[PATCH v2 2/3] dt-bindings: hwmon: Add the +vs supply to the lm75 bindings
Some boards might have a regulator that control the +VS supply, add it
to the bindings.
Signed-off-by: Alban Bedel
---
v2: Removed the unneeded `maxItems` attribute
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index 203829c6ba6f..1c1ce4cf1616 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -43,6 +43,9 @@ properties:
reg:
maxItems: 1
+ vs-supply:
+description: phandle to the regulator that provides the +VS supply
+
required:
- compatible
- reg
@@ -56,5 +59,6 @@ examples:
sensor@48 {
compatible = "st,stlm75";
reg = <0x48>;
+vs-supply = <>;
};
};
--
2.25.1
Re: [PATCH 3/3] hwmon: (lm75) Add regulator support
On Thu, 2020-09-17 at 22:33 -0700, Guenter Roeck wrote:
> On 9/17/20 3:18 AM, Alban Bedel wrote:
> > Add regulator support for boards where the sensor first need to be
> > powered up before it can be used.
> >
> > Signed-off-by: Alban Bedel
> > ---
> > drivers/hwmon/lm75.c | 31 +--
> > 1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > index ba0be48aeadd..b673f8d2ef20 100644
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "lm75.h"
> >
> > /*
> > @@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = {
> > 0x48, 0x49, 0x4a, 0x4b, 0x4c,
> > struct lm75_data {
> > struct i2c_client *client;
> > struct regmap *regmap;
> > + struct regulator*vs;
> > u8 orig_conf;
> > u8 current_conf;
> > u8 resolution; /* In bits, 9 to 16 */
> > @@ -540,6 +542,8 @@ static void lm75_remove(void *data)
> > struct i2c_client *client = lm75->client;
> >
> > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
> > + if (lm75->vs)
> > + regulator_disable(lm75->vs);
> > }
> >
> > static int
> > @@ -567,6 +571,14 @@ lm75_probe(struct i2c_client *client, const
> > struct i2c_device_id *id)
> > data->client = client;
> > data->kind = kind;
> >
> > + data->vs = devm_regulator_get_optional(dev, "vs");
>
> Looking into the regulator API, it may be better if you use
> devm_regulator_get().
> AFAICS it returns a dummy regulator if there is none, and NULL if the
> regulator subsystem is disabled. So
> data->vs = devm_regulator_get(dev, "vs");
> if (IS_ERR(data->vs))
> return PTR_ERR(data->vs);
> should work and would be less messy.
Ok, I'll change that in the next version.
> > + if (IS_ERR(data->vs)) {
> > + if (PTR_ERR(data->vs) == -ENODEV)
> > + data->vs = NULL;
> > + else
> > + return PTR_ERR(data->vs);
> > + }
> > +
> > data->regmap = devm_regmap_init_i2c(client,
> > _regmap_config);
> > if (IS_ERR(data->regmap))
> > return PTR_ERR(data->regmap);
> > @@ -581,11 +593,21 @@ lm75_probe(struct i2c_client *client, const
> > struct i2c_device_id *id)
> > data->sample_time = data->params->default_sample_time;
> > data->resolution = data->params->default_resolution;
> >
> > + /* Enable the power */
> > + if (data->vs) {
> > + err = regulator_enable(data->vs);
> > + if (err) {
> > + dev_err(dev, "failed to enable regulator:
> > %d\n", err);
> > + return err;
> > + }
> > + }
> > +
>
> How about device removal ? Don't you have to call regulator_disable()
> there as well ? If so, it might be best to use
> devm_add_action_or_reset() to register a disable function.
This is handled in lm75_remove() where I added the regulator_disable()
call.
Alban
signature.asc
Description: This is a digitally signed message part
[PATCH 2/3] dt-bindings: hwmon: Add the +vs supply to the lm75 bindings
Some boards might have a regulator that control the +VS supply, add it
to the bindings.
Signed-off-by: Alban Bedel
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 5 +
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index 78d6291fcdcc..1a4405b17924 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -43,6 +43,10 @@ properties:
reg:
maxItems: 1
+ vs-supply:
+maxItems: 1
+description: phandle to the regulator that provides the +VS supply
+
required:
- compatible
- reg
@@ -52,4 +56,5 @@ examples:
sensor@48 {
compatible = "st,stlm75";
reg = <0x48>;
+vs-supply = <>;
};
--
2.25.1
[PATCH 1/3] dt-bindings: hwmon: Convert lm75 bindings to yaml
In order to automate the verification of DT nodes convert lm75.txt to
lm75.yaml.
Signed-off-by: Alban Bedel
---
.../devicetree/bindings/hwmon/lm75.txt| 39 -
.../devicetree/bindings/hwmon/lm75.yaml | 55 +++
2 files changed, 55 insertions(+), 39 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt
create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.txt
b/Documentation/devicetree/bindings/hwmon/lm75.txt
deleted file mode 100644
index 273616702c51..
--- a/Documentation/devicetree/bindings/hwmon/lm75.txt
+++ /dev/null
@@ -1,39 +0,0 @@
-*LM75 hwmon sensor.
-
-Required properties:
-- compatible: manufacturer and chip name, one of
- "adi,adt75",
- "dallas,ds1775",
- "dallas,ds75",
- "dallas,ds7505",
- "gmt,g751",
- "national,lm75",
- "national,lm75a",
- "national,lm75b",
- "maxim,max6625",
- "maxim,max6626",
- "maxim,max31725",
- "maxim,max31726",
- "maxim,mcp980x",
- "nxp,pct2075",
- "st,stds75",
- "st,stlm75",
- "microchip,tcn75",
- "ti,tmp100",
- "ti,tmp101",
- "ti,tmp105",
- "ti,tmp112",
- "ti,tmp175",
- "ti,tmp275",
- "ti,tmp75",
- "ti,tmp75b",
- "ti,tmp75c",
-
-- reg: I2C bus address of the device
-
-Example:
-
-sensor@48 {
- compatible = "st,stlm75";
- reg = <0x48>;
-};
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
new file mode 100644
index ..78d6291fcdcc
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/lm75.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LM75 hwmon sensor
+
+maintainers:
+ - Jean Delvare
+ - Guenter Roeck
+
+properties:
+ compatible:
+enum:
+ - adi,adt75
+ - dallas,ds1775
+ - dallas,ds75
+ - dallas,ds7505
+ - gmt,g751
+ - national,lm75
+ - national,lm75a
+ - national,lm75b
+ - maxim,max6625
+ - maxim,max6626
+ - maxim,max31725
+ - maxim,max31726
+ - maxim,mcp980x
+ - nxp,pct2075
+ - st,stds75
+ - st,stlm75
+ - microchip,tcn75
+ - ti,tmp100
+ - ti,tmp101
+ - ti,tmp105
+ - ti,tmp112
+ - ti,tmp175
+ - ti,tmp275
+ - ti,tmp75
+ - ti,tmp75b
+ - ti,tmp75c
+
+ reg:
+maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+sensor@48 {
+compatible = "st,stlm75";
+reg = <0x48>;
+};
--
2.25.1
[PATCH 3/3] hwmon: (lm75) Add regulator support
Add regulator support for boards where the sensor first need to be
powered up before it can be used.
Signed-off-by: Alban Bedel
---
drivers/hwmon/lm75.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index ba0be48aeadd..b673f8d2ef20 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "lm75.h"
/*
@@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { 0x48, 0x49,
0x4a, 0x4b, 0x4c,
struct lm75_data {
struct i2c_client *client;
struct regmap *regmap;
+ struct regulator*vs;
u8 orig_conf;
u8 current_conf;
u8 resolution; /* In bits, 9 to 16 */
@@ -540,6 +542,8 @@ static void lm75_remove(void *data)
struct i2c_client *client = lm75->client;
i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
+ if (lm75->vs)
+ regulator_disable(lm75->vs);
}
static int
@@ -567,6 +571,14 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->client = client;
data->kind = kind;
+ data->vs = devm_regulator_get_optional(dev, "vs");
+ if (IS_ERR(data->vs)) {
+ if (PTR_ERR(data->vs) == -ENODEV)
+ data->vs = NULL;
+ else
+ return PTR_ERR(data->vs);
+ }
+
data->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(data->regmap))
return PTR_ERR(data->regmap);
@@ -581,11 +593,21 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
data->sample_time = data->params->default_sample_time;
data->resolution = data->params->default_resolution;
+ /* Enable the power */
+ if (data->vs) {
+ err = regulator_enable(data->vs);
+ if (err) {
+ dev_err(dev, "failed to enable regulator: %d\n", err);
+ return err;
+ }
+ }
+
/* Cache original configuration */
status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
if (status < 0) {
dev_dbg(dev, "Can't read config? %d\n", status);
- return status;
+ err = status;
+ goto disable_regulator;
}
data->orig_conf = status;
data->current_conf = status;
@@ -593,7 +615,7 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
err = lm75_write_config(data, data->params->set_mask,
data->params->clr_mask);
if (err)
- return err;
+ goto disable_regulator;
err = devm_add_action_or_reset(dev, lm75_remove, data);
if (err)
@@ -608,6 +630,11 @@ lm75_probe(struct i2c_client *client, const struct
i2c_device_id *id)
dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
return 0;
+
+disable_regulator:
+ if (data->vs)
+ regulator_disable(data->vs);
+ return err;
}
static const struct i2c_device_id lm75_ids[] = {
--
2.25.1
[PATCH 0/3] hwmon: (lm75) Add regulator support
Hi everybody, this small series add regulator support to the lm75 driver for boards that don't always power such a sensor. While at it also convert the DT bindings to yaml. Alban Bedel (3): dt-bindings: hwmon: Convert lm75 bindings to yaml dt-bindings: hwmon: Add the +vs supply to the lm75 bindings hwmon: (lm75) Add regulator support .../devicetree/bindings/hwmon/lm75.txt| 39 .../devicetree/bindings/hwmon/lm75.yaml | 60 +++ drivers/hwmon/lm75.c | 31 +- 3 files changed, 89 insertions(+), 41 deletions(-) delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml -- 2.25.1
[PATCH bpf-next v1 5/7] tools: bpftool: support loading map by fd from parent process
From: Alban Crequy
Using a file descriptor passed by the parent process enables
applications to fork a bpftool command to inspect a map they know by
file descriptor even when they don't support bpffs or map ids.
Documentation and bash completion updated as well.
Signed-off-by: Alban Crequy
---
tools/bpf/bpftool/Documentation/bpftool-map.rst | 8
tools/bpf/bpftool/bash-completion/bpftool | 10 +-
tools/bpf/bpftool/map.c | 13 +
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst
b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index dfd8352fa453..658fe2fb8ecf 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -265,6 +265,14 @@ would be lost as soon as bpftool exits).
anon_inode:bpf-map
+**# bpftool map exec pinned /sys/fs/bpf/foo fd 99 cmd -- bpftool map show fd
99**
+
+::
+
+ 10: hash name some_map flags 0x0
+ key 4B value 8B max_entries 2048 memlock 167936B
+
+
SEE ALSO
**bpf**\ (2),
diff --git a/tools/bpf/bpftool/bash-completion/bpftool
b/tools/bpf/bpftool/bash-completion/bpftool
index 63cd53c4d3a7..9ec1833bda1e 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -253,7 +253,7 @@ _bpftool()
esac
local PROG_TYPE='id pinned tag'
-local MAP_TYPE='id pinned'
+local MAP_TYPE='id pinned fd'
case $command in
show|list)
[[ $prev != "$command" ]] && return 0
@@ -343,7 +343,7 @@ _bpftool()
obj=${words[3]}
if [[ ${words[-4]} == "map" ]]; then
-COMPREPLY=( $( compgen -W "id pinned" -- "$cur" ) )
+COMPREPLY=( $( compgen -W "id pinned fd" -- "$cur" ) )
return 0
fi
if [[ ${words[-3]} == "map" ]]; then
@@ -406,7 +406,7 @@ _bpftool()
esac
;;
map)
-local MAP_TYPE='id pinned'
+local MAP_TYPE='id pinned fd'
case $command in
show|list|dump|peek|pop|dequeue)
case $prev in
@@ -462,7 +462,7 @@ _bpftool()
return 0
;;
innermap)
-COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" )
)
+COMPREPLY+=( $( compgen -W "id pinned fd" --
"$cur" ) )
return 0
;;
id)
@@ -525,7 +525,7 @@ _bpftool()
# map, depending on the type of the map to update.
case "$(_bpftool_map_guess_map_type)" in
array_of_maps|hash_of_maps)
-local MAP_TYPE='id pinned'
+local MAP_TYPE='id pinned fd'
COMPREPLY+=( $( compgen -W "$MAP_TYPE" \
-- "$cur" ) )
return 0
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 768497364cee..9befcabc299d 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -125,6 +125,19 @@ int map_parse_fd(int *argc, char ***argv)
NEXT_ARGP();
return open_obj_pinned_any(path, BPF_OBJ_MAP);
+ } else if (is_prefix(**argv, "fd")) {
+ char *endptr;
+
+ NEXT_ARGP();
+
+ fd = strtoul(**argv, , 0);
+ if (*endptr) {
+ p_err("can't parse %s as fd", **argv);
+ return -1;
+ }
+ NEXT_ARGP();
+
+ return dup(fd);
}
p_err("expected 'id' or 'pinned', got: '%s'?", **argv);
--
2.20.1
Re: [PATCH bpf-next v1] tools/bpftool: create map of maps
Thanks for the reviews, Jakub and Quentin! I will address it and
resend a new version when bpf-next opens again.
I'm also preparing some other patches on "bpftool map" about pinning
and passing file descriptors to help applications that don't support
map pinning directly.
On Tue, Mar 5, 2019 at 6:32 PM Jakub Kicinski
wrote:
>
> On Tue, 5 Mar 2019 17:38:03 +0100, Alban Crequy wrote:
> > From: Alban Crequy
> >
> > Before this patch, there was no way to fill attr.inner_map_fd, necessary
> > for array_of_maps or hash_of_maps.
> >
> > This patch adds keyword 'innermap' to pass the innermap, either as an id
> > or as a pinned map.
> >
> > Example of commands:
> >
> > $ sudo bpftool map create /sys/fs/bpf/innermap type hash \
> > key 8 value 8 entries 64 name innermap flags 1
> > $ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
> > innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
> > entries 64 name myoutermap flags 1
> > $ sudo bpftool map show pinned /sys/fs/bpf/outermap
> > 47: hash_of_maps name myoutermap flags 0x1
> > key 64B value 4B max_entries 64 memlock 12288B
> >
> > Documentation and bash completion updated as well.
> >
> > Signed-off-by: Alban Crequy
>
> bpf-next is closed let's continue reviewing, but you'll probably have
> to repost after the merge window :(
>
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index e0c650d91784..7d8ce903a471 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -1151,6 +1151,9 @@ static int do_create(int argc, char **argv)
> > return -1;
> > }
> > NEXT_ARG();
> > + } else if (is_prefix(*argv, "innermap")) {
> > + NEXT_ARG();
> > + attr.inner_map_fd = map_parse_fd(, );
>
> You need to check if the return value is not -1, and also close this
> file descriptor (a) when done, (b) when error happens.
>
> > }
> > }
> >
[PATCH bpf-next v1] tools/bpftool: create map of maps
From: Alban Crequy
Before this patch, there was no way to fill attr.inner_map_fd, necessary
for array_of_maps or hash_of_maps.
This patch adds keyword 'innermap' to pass the innermap, either as an id
or as a pinned map.
Example of commands:
$ sudo bpftool map create /sys/fs/bpf/innermap type hash \
key 8 value 8 entries 64 name innermap flags 1
$ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
entries 64 name myoutermap flags 1
$ sudo bpftool map show pinned /sys/fs/bpf/outermap
47: hash_of_maps name myoutermap flags 0x1
key 64B value 4B max_entries 64 memlock 12288B
Documentation and bash completion updated as well.
Signed-off-by: Alban Crequy
---
tools/bpf/bpftool/Documentation/bpftool-prog.rst | 2 +-
tools/bpf/bpftool/bash-completion/bpftool| 13 +
tools/bpf/bpftool/map.c | 5 -
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 9386bd6e0396..35902ab95cc5 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -25,7 +25,7 @@ PROG COMMANDS
| **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**
| **visual** | **linum**}]
| **bpftool** **prog dump jited** *PROG* [{**file** *FILE* | **opcodes**
| **linum**}]
| **bpftool** **prog pin** *PROG* *FILE*
-| **bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*]
[**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+| **bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*]
[**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
[**innermap** MAP]
| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog tracelog**
diff --git a/tools/bpf/bpftool/bash-completion/bpftool
b/tools/bpf/bpftool/bash-completion/bpftool
index b803827d01e8..dcdc39510dc3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -461,6 +461,18 @@ _bpftool()
_sysfs_get_netdevs
return 0
;;
+innermap)
+COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" )
)
+return 0
+;;
+id)
+_bpftool_get_map_ids
+return 0
+;;
+pinned)
+_filedir
+return 0
+;;
*)
_bpftool_once_attr 'type'
_bpftool_once_attr 'key'
@@ -469,6 +481,7 @@ _bpftool()
_bpftool_once_attr 'name'
_bpftool_once_attr 'flags'
_bpftool_once_attr 'dev'
+_bpftool_once_attr 'innermap'
return 0
;;
esac
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index e0c650d91784..7d8ce903a471 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1151,6 +1151,9 @@ static int do_create(int argc, char **argv)
return -1;
}
NEXT_ARG();
+ } else if (is_prefix(*argv, "innermap")) {
+ NEXT_ARG();
+ attr.inner_map_fd = map_parse_fd(, );
}
}
@@ -1231,7 +1234,7 @@ static int do_help(int argc, char **argv)
"Usage: %s %s { show | list } [MAP]\n"
" %s %s create FILE type TYPE key KEY_SIZE value
VALUE_SIZE \\\n"
" entries MAX_ENTRIES name NAME
[flags FLAGS] \\\n"
- " [dev NAME]\n"
+ " [dev NAME] [innermap MAP]\n"
" %s %s dump MAP\n"
" %s %s update MAP [key DATA] [value VALUE]
[UPDATE_FLAGS]\n"
" %s %s lookup MAP [key DATA]\n"
--
2.20.1
Re: [PATCH bpf v2] bpf, lpm: fix lookup bug in map_delete_elem
adding cc: Craig Gallek
I got the "git send-email" command wrong, and the cc to Craig was
removed, despite being listed in the commitmsg. Sorry for my mistake.
There is a copy of the patch at this URL, if needed:
https://github.com/kinvolk/linux/commit/5c522b02ee447f2eb060f840ba709af6b425f932
On Fri, Feb 22, 2019 at 2:19 PM Alban Crequy wrote:
>
> From: Alban Crequy
>
> trie_delete_elem() was deleting an entry even though it was not matching
> if the prefixlen was correct. This patch adds a check on matchlen.
>
> Reproducer:
>
> $ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1
> entries 128 name mylpm flags 1
> $ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa bb
> cc dd value hex 01
> $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
> key: 10 00 00 00 aa bb cc dd value: 01
> Found 1 element
> $ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff ff
> ff ff
> $ echo $?
> 0
> $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
> Found 0 elements
>
> A similar reproducer is added in the selftests.
>
> Without the patch:
>
> $ sudo ./tools/testing/selftests/bpf/test_lpm_map
> test_lpm_map: test_lpm_map.c:485: test_lpm_delete: Assertion
> `bpf_map_delete_elem(map_fd, key) == -1 && errno == ENOENT' failed.
> Aborted
>
> With the patch: test_lpm_map runs without errors.
>
> Fixes: e454cf595853 ("bpf: Implement map_delete_elem for
> BPF_MAP_TYPE_LPM_TRIE")
> Cc: Craig Gallek
> Signed-off-by: Alban Crequy
>
> ---
>
> Changes v1 to v2:
> - add selftest (review from Martin)
> - update commitmsg tags (review from Martin)
> - rebase on "bpf" tree and test again (review from Martin)
> ---
> kernel/bpf/lpm_trie.c | 1 +
> tools/testing/selftests/bpf/test_lpm_map.c | 10 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
> index abf1002080df..93a5cbbde421 100644
> --- a/kernel/bpf/lpm_trie.c
> +++ b/kernel/bpf/lpm_trie.c
> @@ -471,6 +471,7 @@ static int trie_delete_elem(struct bpf_map *map, void
> *_key)
> }
>
> if (!node || node->prefixlen != key->prefixlen ||
> + node->prefixlen != matchlen ||
> (node->flags & LPM_TREE_NODE_FLAG_IM)) {
> ret = -ENOENT;
> goto out;
> diff --git a/tools/testing/selftests/bpf/test_lpm_map.c
> b/tools/testing/selftests/bpf/test_lpm_map.c
> index 147e34cfceb7..02d7c871862a 100644
> --- a/tools/testing/selftests/bpf/test_lpm_map.c
> +++ b/tools/testing/selftests/bpf/test_lpm_map.c
> @@ -474,6 +474,16 @@ static void test_lpm_delete(void)
> assert(bpf_map_lookup_elem(map_fd, key, ) == -1 &&
> errno == ENOENT);
>
> + key->prefixlen = 30; // unused prefix so far
> + inet_pton(AF_INET, "192.255.0.0", key->data);
> + assert(bpf_map_delete_elem(map_fd, key) == -1 &&
> + errno == ENOENT);
> +
> + key->prefixlen = 16; // same prefix as the root node
> + inet_pton(AF_INET, "192.255.0.0", key->data);
> + assert(bpf_map_delete_elem(map_fd, key) == -1 &&
> + errno == ENOENT);
> +
> /* assert initial lookup */
> key->prefixlen = 32;
> inet_pton(AF_INET, "192.168.0.1", key->data);
> --
> 2.20.1
>
[PATCH bpf v2] bpf, lpm: fix lookup bug in map_delete_elem
From: Alban Crequy
trie_delete_elem() was deleting an entry even though it was not matching
if the prefixlen was correct. This patch adds a check on matchlen.
Reproducer:
$ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1 entries
128 name mylpm flags 1
$ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa bb cc
dd value hex 01
$ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
key: 10 00 00 00 aa bb cc dd value: 01
Found 1 element
$ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff ff ff
ff
$ echo $?
0
$ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
Found 0 elements
A similar reproducer is added in the selftests.
Without the patch:
$ sudo ./tools/testing/selftests/bpf/test_lpm_map
test_lpm_map: test_lpm_map.c:485: test_lpm_delete: Assertion
`bpf_map_delete_elem(map_fd, key) == -1 && errno == ENOENT' failed.
Aborted
With the patch: test_lpm_map runs without errors.
Fixes: e454cf595853 ("bpf: Implement map_delete_elem for BPF_MAP_TYPE_LPM_TRIE")
Cc: Craig Gallek
Signed-off-by: Alban Crequy
---
Changes v1 to v2:
- add selftest (review from Martin)
- update commitmsg tags (review from Martin)
- rebase on "bpf" tree and test again (review from Martin)
---
kernel/bpf/lpm_trie.c | 1 +
tools/testing/selftests/bpf/test_lpm_map.c | 10 ++
2 files changed, 11 insertions(+)
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index abf1002080df..93a5cbbde421 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -471,6 +471,7 @@ static int trie_delete_elem(struct bpf_map *map, void *_key)
}
if (!node || node->prefixlen != key->prefixlen ||
+ node->prefixlen != matchlen ||
(node->flags & LPM_TREE_NODE_FLAG_IM)) {
ret = -ENOENT;
goto out;
diff --git a/tools/testing/selftests/bpf/test_lpm_map.c
b/tools/testing/selftests/bpf/test_lpm_map.c
index 147e34cfceb7..02d7c871862a 100644
--- a/tools/testing/selftests/bpf/test_lpm_map.c
+++ b/tools/testing/selftests/bpf/test_lpm_map.c
@@ -474,6 +474,16 @@ static void test_lpm_delete(void)
assert(bpf_map_lookup_elem(map_fd, key, ) == -1 &&
errno == ENOENT);
+ key->prefixlen = 30; // unused prefix so far
+ inet_pton(AF_INET, "192.255.0.0", key->data);
+ assert(bpf_map_delete_elem(map_fd, key) == -1 &&
+ errno == ENOENT);
+
+ key->prefixlen = 16; // same prefix as the root node
+ inet_pton(AF_INET, "192.255.0.0", key->data);
+ assert(bpf_map_delete_elem(map_fd, key) == -1 &&
+ errno == ENOENT);
+
/* assert initial lookup */
key->prefixlen = 32;
inet_pton(AF_INET, "192.168.0.1", key->data);
--
2.20.1
Re: [PATCH bpf-next v1] bpf, lpm: fix lookup bug in map_delete_elem
On Thu, Feb 21, 2019 at 11:24 PM Martin Lau wrote:
>
> On Thu, Feb 21, 2019 at 05:39:26PM +0100, Alban Crequy wrote:
> > From: Alban Crequy
> >
> > trie_delete_elem() was deleting an entry even though it was not matching
> > if the prefixlen was correct. This patch adds a check on matchlen.
> >
> > Reproducer:
> >
> > $ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1
> > entries 128 name mylpm flags 1
> > $ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa
> > bb cc dd value hex 01
> > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
> > key: 10 00 00 00 aa bb cc dd value: 01
> > Found 1 element
> > $ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff
> > ff ff ff
> > $ echo $?
> > 0
> > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
> > Found 0 elements
> The change makes sense to me. Can you add this reproducer to
> tools/testing/selftests/bpf/test_lpm_map.c?
>
> Bug fix should be for the "bpf" tree instead of "bpf-next"
> Fixes tag is also required, like
>
> Fixes: e454cf595853 ("bpf: Implement map_delete_elem for
> BPF_MAP_TYPE_LPM_TRIE")
> Cc: Craig Gallek
Thanks! I'll send a v2 shortly with the selftest and the tags, based
on "bpf" tree.
Cheers,
Alban
> > Signed-off-by: Alban Crequy
> > ---
> > kernel/bpf/lpm_trie.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
> > index abf1002080df..93a5cbbde421 100644
> > --- a/kernel/bpf/lpm_trie.c
> > +++ b/kernel/bpf/lpm_trie.c
> > @@ -471,6 +471,7 @@ static int trie_delete_elem(struct bpf_map *map, void
> > *_key)
> > }
> >
> > if (!node || node->prefixlen != key->prefixlen ||
> > + node->prefixlen != matchlen ||
> > (node->flags & LPM_TREE_NODE_FLAG_IM)) {
> > ret = -ENOENT;
> > goto out;
> > --
> > 2.20.1
> >
[PATCH bpf-next v1] bpf, lpm: fix lookup bug in map_delete_elem
From: Alban Crequy
trie_delete_elem() was deleting an entry even though it was not matching
if the prefixlen was correct. This patch adds a check on matchlen.
Reproducer:
$ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1 entries
128 name mylpm flags 1
$ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa bb cc
dd value hex 01
$ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
key: 10 00 00 00 aa bb cc dd value: 01
Found 1 element
$ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff ff ff
ff
$ echo $?
0
$ sudo bpftool map dump pinned /sys/fs/bpf/mylpm
Found 0 elements
Signed-off-by: Alban Crequy
---
kernel/bpf/lpm_trie.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index abf1002080df..93a5cbbde421 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -471,6 +471,7 @@ static int trie_delete_elem(struct bpf_map *map, void *_key)
}
if (!node || node->prefixlen != key->prefixlen ||
+ node->prefixlen != matchlen ||
(node->flags & LPM_TREE_NODE_FLAG_IM)) {
ret = -ENOENT;
goto out;
--
2.20.1
[PATCH bpf-next v2] bpf: bpftool, fix documentation for attach types
From: Alban Crequy
bpftool has support for attach types "stream_verdict" and
"stream_parser" but the documentation was referring to them as
"skb_verdict" and "skb_parse". The inconsistency comes from commit
b7d3826c2ed6 ("bpf: bpftool, add support for attaching programs to
maps").
This patch changes the documentation to match the implementation:
- "bpftool prog help"
- man pages
- bash completion
Signed-off-by: Alban Crequy
---
Changes v1 to v2:
- fix man pages & bash completion (from Quentin's review)
---
tools/bpf/bpftool/Documentation/bpftool-prog.rst | 2 +-
tools/bpf/bpftool/bash-completion/bpftool| 4 ++--
tools/bpf/bpftool/prog.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 7e59495cb028..12bc1e2d4b46 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -42,7 +42,7 @@ PROG COMMANDS
| **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
| **cgroup/sendmsg6**
| }
| *ATTACH_TYPE* := {
-| **msg_verdict** | **skb_verdict** | **skb_parse** |
**flow_dissector**
+| **msg_verdict** | **stream_verdict** | **stream_parser** |
**flow_dissector**
| }
diff --git a/tools/bpf/bpftool/bash-completion/bpftool
b/tools/bpf/bpftool/bash-completion/bpftool
index 763dd12482aa..b803827d01e8 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -311,8 +311,8 @@ _bpftool()
return 0
;;
5)
-COMPREPLY=( $( compgen -W 'msg_verdict skb_verdict
\
-skb_parse flow_dissector' -- "$cur" ) )
+COMPREPLY=( $( compgen -W 'msg_verdict
stream_verdict \
+stream_parser flow_dissector' -- "$cur" ) )
return 0
;;
6)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 33ed0806ccc0..db978c8d76a8 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1199,7 +1199,7 @@ static int do_help(int argc, char **argv)
" cgroup/bind4 | cgroup/bind6 |
cgroup/post_bind4 |\n"
" cgroup/post_bind6 | cgroup/connect4 |
cgroup/connect6 |\n"
" cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
- " ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse
|\n"
+ " ATTACH_TYPE := { msg_verdict | stream_verdict |
stream_parser |\n"
"flow_dissector }\n"
" " HELP_SPEC_OPTIONS "\n"
"",
--
2.20.1
Re: [PATCH] bpf: bpftool, fix documentation for attach types
On Mon, Feb 11, 2019 at 2:26 PM Quentin Monnet
wrote:
>
> 2019-02-11 13:54 UTC+0100 ~ Alban Crequy
> > From: Alban Crequy
> >
> > bpftool has support for attach types "stream_verdict" and
> > "stream_parser" but the documentation was referring to them with
> > "skb_verdict" and "skb_parse". The inconsistency comes from commit
> > b7d3826c2ed6 ("bpf: bpftool, add support for attaching programs to
> > maps").
> >
> > This patch changes the documentation to match the implementation.
> >
> > Signed-off-by: Alban Crequy
> > ---
> > tools/bpf/bpftool/prog.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index 0640e9bc0ada..dfaa019a60f0 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -1198,7 +1198,7 @@ static int do_help(int argc, char **argv)
> > " cgroup/bind4 | cgroup/bind6 |
> > cgroup/post_bind4 |\n"
> > " cgroup/post_bind6 | cgroup/connect4 |
> > cgroup/connect6 |\n"
> > " cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
> > - " ATTACH_TYPE := { msg_verdict | skb_verdict |
> > skb_parse |\n"
> > + " ATTACH_TYPE := { msg_verdict | stream_verdict |
> > stream_parser |\n"
> > " flow_dissector }\n"
> > " " HELP_SPEC_OPTIONS "\n"
> > "",
> >
>
> Thanks Alban!
>
> Could you please fix the man page and the bash completion file at the
> same time?
Yes, I will do that soon. Sorry for the delay in replying.
Thanks,
Alban
[PATCH] bpf: bpftool, fix documentation for attach types
From: Alban Crequy
bpftool has support for attach types "stream_verdict" and
"stream_parser" but the documentation was referring to them with
"skb_verdict" and "skb_parse". The inconsistency comes from commit
b7d3826c2ed6 ("bpf: bpftool, add support for attaching programs to
maps").
This patch changes the documentation to match the implementation.
Signed-off-by: Alban Crequy
---
tools/bpf/bpftool/prog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 0640e9bc0ada..dfaa019a60f0 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1198,7 +1198,7 @@ static int do_help(int argc, char **argv)
" cgroup/bind4 | cgroup/bind6 |
cgroup/post_bind4 |\n"
" cgroup/post_bind6 | cgroup/connect4 |
cgroup/connect6 |\n"
" cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
- " ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse
|\n"
+ " ATTACH_TYPE := { msg_verdict | stream_verdict |
stream_parser |\n"
"flow_dissector }\n"
" " HELP_SPEC_OPTIONS "\n"
"",
--
2.20.1
[PATCH 6/6] MAINTAINERS: Add an entry for the ath79 SPI driver
Add myself as the maintainer for the ath79 SPI driver. Signed-off-by: Alban Bedel --- MAINTAINERS | 9 + 1 file changed, 9 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 71e2965fc656..a03ab41489cf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2520,6 +2520,15 @@ S: Maintained F: drivers/gpio/gpio-ath79.c F: Documentation/devicetree/bindings/gpio/gpio-ath79.txt +ATHEROS 71XX/9XXX SPI DRIVER +M: Alban Bedel +W: https://github.com/AlbanBedel/linux +T: git git://github.com/AlbanBedel/linux +S: Maintained +F: drivers/spi/spi-ath79.c +F: include/linux/platform_data/spi-ath79.h +F: Documentation/devicetree/bindings/spi/spi-ath79.txt + ATHEROS 71XX/9XXX USB PHY DRIVER M: Alban Bedel W: https://github.com/AlbanBedel/linux -- 2.19.1
[PATCH 3/6] spi: ath79: Enable support for compile test
To allow building this driver in compile test we need to remove all
dependency on headers from arch/mips/include. To allow this we
explicitly define all the registers locally instead of using
ar71xx_regs.h and we move the platform data struct definition to
include/linux/platform_data/spi-ath79.h.
Signed-off-by: Alban Bedel
---
arch/mips/ath79/dev-spi.h | 2 +-
drivers/spi/Kconfig | 2 +-
drivers/spi/spi-ath79.c | 15 ---
.../linux/platform_data/spi-ath79.h | 0
4 files changed, 14 insertions(+), 5 deletions(-)
rename arch/mips/include/asm/mach-ath79/ath79_spi_platform.h =>
include/linux/platform_data/spi-ath79.h (100%)
diff --git a/arch/mips/ath79/dev-spi.h b/arch/mips/ath79/dev-spi.h
index d732565ca736..6e15bc8651be 100644
--- a/arch/mips/ath79/dev-spi.h
+++ b/arch/mips/ath79/dev-spi.h
@@ -13,7 +13,7 @@
#define _ATH79_DEV_SPI_H
#include
-#include
+#include
void ath79_register_spi(struct ath79_spi_platform_data *pdata,
struct spi_board_info const *info,
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 128892c7e21e..71d3d2d5e5d1 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -63,7 +63,7 @@ config SPI_ALTERA
config SPI_ATH79
tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
- depends on ATH79
+ depends on ATH79 || COMPILE_TEST
select SPI_BITBANG
help
This enables support for the SPI controller present on the
diff --git a/drivers/spi/spi-ath79.c b/drivers/spi/spi-ath79.c
index edf695a359f4..09c4fb7fcf7a 100644
--- a/drivers/spi/spi-ath79.c
+++ b/drivers/spi/spi-ath79.c
@@ -23,15 +23,24 @@
#include
#include
#include
-
-#include
-#include
+#include
#define DRV_NAME "ath79-spi"
#define ATH79_SPI_RRW_DELAY_FACTOR 12000
#define MHZ(1000 * 1000)
+#define AR71XX_SPI_REG_FS 0x00/* Function Select */
+#define AR71XX_SPI_REG_CTRL0x04/* SPI Control */
+#define AR71XX_SPI_REG_IOC 0x08/* SPI I/O Control */
+#define AR71XX_SPI_REG_RDS 0x0c/* Read Data Shift */
+
+#define AR71XX_SPI_FS_GPIO BIT(0) /* Enable GPIO mode */
+
+#define AR71XX_SPI_IOC_DO BIT(0) /* Data Out pin */
+#define AR71XX_SPI_IOC_CLK BIT(8) /* CLK pin */
+#define AR71XX_SPI_IOC_CS(n) BIT(16 + (n))
+
struct ath79_spi {
struct spi_bitbang bitbang;
u32 ioc_base;
diff --git a/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
b/include/linux/platform_data/spi-ath79.h
similarity index 100%
rename from arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
rename to include/linux/platform_data/spi-ath79.h
--
2.19.1
[PATCH 5/6] spi: ath79: Remove some useless includes
Several include are not needed, remove them to keep the list clean. Signed-off-by: Alban Bedel --- drivers/spi/spi-ath79.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/spi/spi-ath79.c b/drivers/spi/spi-ath79.c index 847f354ebef1..257acfb07819 100644 --- a/drivers/spi/spi-ath79.c +++ b/drivers/spi/spi-ath79.c @@ -12,15 +12,12 @@ * */ -#include #include #include -#include #include #include #include #include -#include #include #include #include -- 2.19.1
[PATCH 4/6] spi: ath79: Remove now useless code
The custom setup/cleanup routines included in the ath79 driver only
take care of setting the initial CS state. However that is already
handled by the bitbang code, so this code can be removed.
Signed-off-by: Alban Bedel
---
drivers/spi/spi-ath79.c | 43 ++---
1 file changed, 2 insertions(+), 41 deletions(-)
diff --git a/drivers/spi/spi-ath79.c b/drivers/spi/spi-ath79.c
index 09c4fb7fcf7a..847f354ebef1 100644
--- a/drivers/spi/spi-ath79.c
+++ b/drivers/spi/spi-ath79.c
@@ -109,44 +109,6 @@ static void ath79_spi_disable(struct ath79_spi *sp)
ath79_spi_wr(sp, AR71XX_SPI_REG_FS, 0);
}
-static int ath79_spi_setup_cs(struct spi_device *spi)
-{
- struct ath79_spi *sp = ath79_spidev_to_sp(spi);
-
- if (!spi->cs_gpiod) {
- u32 cs_bit = AR71XX_SPI_IOC_CS(spi->chip_select);
-
- if (spi->mode & SPI_CS_HIGH)
- sp->ioc_base &= ~cs_bit;
- else
- sp->ioc_base |= cs_bit;
-
- ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
- }
-
- return 0;
-}
-
-static int ath79_spi_setup(struct spi_device *spi)
-{
- int status = 0;
-
- if (!spi->controller_state) {
- status = ath79_spi_setup_cs(spi);
- if (status)
- return status;
- }
-
- status = spi_bitbang_setup(spi);
-
- return status;
-}
-
-static void ath79_spi_cleanup(struct spi_device *spi)
-{
- spi_bitbang_cleanup(spi);
-}
-
static u32 ath79_spi_txrx_mode0(struct spi_device *spi, unsigned int nsecs,
u32 word, u8 bits, unsigned flags)
{
@@ -199,8 +161,8 @@ static int ath79_spi_probe(struct platform_device *pdev)
master->use_gpio_descriptors = true;
master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
- master->setup = ath79_spi_setup;
- master->cleanup = ath79_spi_cleanup;
+ master->setup = spi_bitbang_setup;
+ master->cleanup = spi_bitbang_cleanup;
if (pdata) {
master->bus_num = pdata->bus_num;
master->num_chipselect = pdata->num_chipselect;
@@ -209,7 +171,6 @@ static int ath79_spi_probe(struct platform_device *pdev)
sp->bitbang.master = master;
sp->bitbang.chipselect = ath79_spi_chipselect;
sp->bitbang.txrx_word[SPI_MODE_0] = ath79_spi_txrx_mode0;
- sp->bitbang.setup_transfer = spi_bitbang_setup_transfer;
sp->bitbang.flags = SPI_CS_HIGH;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
--
2.19.1
[PATCH 2/6] spi: ath79: Simplify ath79_spi_chipselect()
First of all this callback was slightly misused to setup the clock
polarity at the beginning of a transfer. Beside being at the wrong
place, it is also useless as only SPI mode 1 is supported. Instead
just make sure the base value used for IOC is suitable to start a
transfer by clearing the clock and data bits during the controller
setup.
This also remove the last direct usage of the GPIO API, so we can
remove the direct dependency on GPIOLIB.
Signed-off-by: Alban Bedel
---
drivers/spi/Kconfig | 2 +-
drivers/spi/spi-ath79.c | 40 +---
2 files changed, 10 insertions(+), 32 deletions(-)
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index dc67eda1788a..128892c7e21e 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -63,7 +63,7 @@ config SPI_ALTERA
config SPI_ATH79
tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
- depends on ATH79 && GPIOLIB
+ depends on ATH79
select SPI_BITBANG
help
This enables support for the SPI controller present on the
diff --git a/drivers/spi/spi-ath79.c b/drivers/spi/spi-ath79.c
index ed1068ac055f..edf695a359f4 100644
--- a/drivers/spi/spi-ath79.c
+++ b/drivers/spi/spi-ath79.c
@@ -21,7 +21,6 @@
#include
#include
#include
-#include
#include
#include
@@ -67,38 +66,14 @@ static void ath79_spi_chipselect(struct spi_device *spi,
int is_active)
{
struct ath79_spi *sp = ath79_spidev_to_sp(spi);
int cs_high = (spi->mode & SPI_CS_HIGH) ? is_active : !is_active;
+ u32 cs_bit = AR71XX_SPI_IOC_CS(spi->chip_select);
- if (is_active) {
- /* set initial clock polarity */
- if (spi->mode & SPI_CPOL)
- sp->ioc_base |= AR71XX_SPI_IOC_CLK;
- else
- sp->ioc_base &= ~AR71XX_SPI_IOC_CLK;
-
- ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
- }
-
- if (spi->cs_gpiod) {
- /*
-* SPI chipselect is normally active-low, but
-* inversion semantics are handled by gpiolib.
-*
-* FIXME: is this ever used? The driver doesn't
-* set SPI_MASTER_GPIO_SS so this callback should not
-* get called if a CS GPIO is found by the SPI core.
-*/
- gpiod_set_value_cansleep(spi->cs_gpiod, is_active);
- } else {
- u32 cs_bit = AR71XX_SPI_IOC_CS(spi->chip_select);
-
- if (cs_high)
- sp->ioc_base |= cs_bit;
- else
- sp->ioc_base &= ~cs_bit;
-
- ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
- }
+ if (cs_high)
+ sp->ioc_base |= cs_bit;
+ else
+ sp->ioc_base &= ~cs_bit;
+ ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
}
static void ath79_spi_enable(struct ath79_spi *sp)
@@ -110,6 +85,9 @@ static void ath79_spi_enable(struct ath79_spi *sp)
sp->reg_ctrl = ath79_spi_rr(sp, AR71XX_SPI_REG_CTRL);
sp->ioc_base = ath79_spi_rr(sp, AR71XX_SPI_REG_IOC);
+ /* clear clk and mosi in the base state */
+ sp->ioc_base &= ~(AR71XX_SPI_IOC_DO | AR71XX_SPI_IOC_CLK);
+
/* TODO: setup speed? */
ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, 0x43);
}
--
2.19.1
[PATCH 1/6] spi: bitbang: Don't call chipselect() in spi_bitbang_setup()
spi_setup() already call spi_set_cs() right after calling the
controller setup method, so there is no need for the bitbang driver to
do that. Because of this the chipselect() callback was confusingly
still called when CS is GPIO based.
Signed-off-by: Alban Bedel
---
drivers/spi/spi-bitbang.c | 13 -
1 file changed, 13 deletions(-)
diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index f29176000b8d..dd9a8c54a693 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -213,19 +213,6 @@ int spi_bitbang_setup(struct spi_device *spi)
dev_dbg(>dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);
- /* NOTE we _need_ to call chipselect() early, ideally with adapter
-* setup, unless the hardware defaults cooperate to avoid confusion
-* between normal (active low) and inverted chipselects.
-*/
-
- /* deselect chip (low or high) */
- mutex_lock(>lock);
- if (!bitbang->busy) {
- bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
- ndelay(cs->nsecs);
- }
- mutex_unlock(>lock);
-
return 0;
}
EXPORT_SYMBOL_GPL(spi_bitbang_setup);
--
2.19.1
Re: [PATCH 3/8] nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional
On Tue, 15 Jan 2019 12:40:53 +
Srinivas Kandagatla wrote:
> On 06/01/2019 19:28, Alban Bedel wrote:
> > Add helper functions to make the driver code simpler when a cell is
> > optional. Using these functions just return NULL when the cell doesn't
> > exists or if nvmem is disabled.
> >
> > Signed-off-by: Alban Bedel
> > ---
> > drivers/nvmem/core.c | 48 ++
> > include/linux/nvmem-consumer.h | 16
> > 2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index f8c43da6f2ca..8e1b52559467 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -1083,6 +1083,30 @@ struct nvmem_cell *nvmem_cell_get(struct device
> > *dev, const char *id)
> > }
> > EXPORT_SYMBOL_GPL(nvmem_cell_get);
> >
> > +/**
> > + * nvmem_cell_get_optional() - Get an optional nvmem cell of device from
> > + * a given id.
> > + *
> > + * @dev: Device that requests the nvmem cell.
> > + * @cell_id: nvmem cell name to get.
> > + *
> > + * Return: Will be NULL if no cell with the given name is defined,
> > + * an ERR_PTR() on error or a valid pointer to a struct nvmem_cell.
> > + * The nvmem_cell will be freed by the nvmem_cell_put().
> > + */
> > +struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
> > + const char *cell_id)
> > +{
> > + struct nvmem_cell *cell;
> > +
> > + cell = nvmem_cell_get(dev, cell_id);
> > + if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
> > + return NULL;
>
> What is the real use-case here, it does not make sense to me to add this
> additional call just to return NULL when cell is not found!
It also return NULL when nvmem is not compiled in. I quiet like such
convenience functions as they make the driver code much simpler and
the intent explicit. It replace:
data->cell = devm_nvmem_cell_get(dev, "my-cell");
if (IS_ERR(data->cell) {
if (PTR_ERR(data->cell) == -ENOENT ||
PTR_ERR(data->cell) == -EOPNOTSUPP)
data->cell = NULL;
else
return PTR_ERR(data->cell);
}
with:
data->cell = dev_nvmem_cell_get_optional(dev, "my-cell");
if (IS_ERR(cell))
return PTR_ERR(data->cell);
It's your call if you find that useful or not.
Alban
pgpIsx0rzWdZb.pgp
Description: OpenPGP digital signature
[PATCH 2/2] phy: ath79-usb: Fix the main reset name to match the DT binding
I submitted this driver several times before it got accepted. The first series hasn't been accepted but the DTS binding did made it. I then made a second series that added generic reset support to the PHY core, this in turn required a change to the DT binding. This second series seemed to have been ignored, so I did a third one without the change to the PHY core and the DT binding update, and this last attempt finally made it. But two months later the DT binding update from the second series has been integrated too. So now the driver doesn't match the binding and the only DTS using it. This patch fix the driver to match the new binding. Signed-off-by: Alban Bedel --- drivers/phy/qualcomm/phy-ath79-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/phy/qualcomm/phy-ath79-usb.c b/drivers/phy/qualcomm/phy-ath79-usb.c index f7d64f3910b4..09a77e556ece 100644 --- a/drivers/phy/qualcomm/phy-ath79-usb.c +++ b/drivers/phy/qualcomm/phy-ath79-usb.c @@ -69,7 +69,7 @@ static int ath79_usb_phy_probe(struct platform_device *pdev) if (!priv) return -ENOMEM; - priv->reset = devm_reset_control_get(>dev, "usb-phy"); + priv->reset = devm_reset_control_get(>dev, "phy"); if (IS_ERR(priv->reset)) return PTR_ERR(priv->reset); -- 2.19.1
[PATCH] MIPS: ath79: Enable OF serial ports in the default config
CONFIG_SERIAL_OF_PLATFORM is needed to get a working console on the OF boards, enable it in the default config to get a working setup out of the box. Signed-off-by: Alban Bedel --- arch/mips/configs/ath79_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/mips/configs/ath79_defconfig b/arch/mips/configs/ath79_defconfig index 4e4ec779f182..6f981af67826 100644 --- a/arch/mips/configs/ath79_defconfig +++ b/arch/mips/configs/ath79_defconfig @@ -66,6 +66,7 @@ CONFIG_SERIAL_8250_CONSOLE=y # CONFIG_SERIAL_8250_PCI is not set CONFIG_SERIAL_8250_NR_UARTS=1 CONFIG_SERIAL_8250_RUNTIME_UARTS=1 +CONFIG_SERIAL_OF_PLATFORM=y CONFIG_SERIAL_AR933X=y CONFIG_SERIAL_AR933X_CONSOLE=y # CONFIG_HW_RANDOM is not set -- 2.19.1
[PATCH 1/2] phy: ath79-usb: Fix the power on error path
In the power on function the error path doesn't return the suspend override to its proper state. It should should deassert this reset line to enable the suspend override. Signed-off-by: Alban Bedel --- drivers/phy/qualcomm/phy-ath79-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/phy/qualcomm/phy-ath79-usb.c b/drivers/phy/qualcomm/phy-ath79-usb.c index 6fd6e07ab345..f7d64f3910b4 100644 --- a/drivers/phy/qualcomm/phy-ath79-usb.c +++ b/drivers/phy/qualcomm/phy-ath79-usb.c @@ -31,7 +31,7 @@ static int ath79_usb_phy_power_on(struct phy *phy) err = reset_control_deassert(priv->reset); if (err && priv->no_suspend_override) - reset_control_assert(priv->no_suspend_override); + reset_control_deassert(priv->no_suspend_override); return err; } -- 2.19.1
[PATCH 1/2] phy: ath79-usb: Fix the power on error path
In the power on function the error path doesn't return the suspend override to its proper state. It should should deassert this reset line to enable the suspend override. Signed-off-by: Alban Bedel --- drivers/phy/qualcomm/phy-ath79-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/phy/qualcomm/phy-ath79-usb.c b/drivers/phy/qualcomm/phy-ath79-usb.c index 6fd6e07ab345..f7d64f3910b4 100644 --- a/drivers/phy/qualcomm/phy-ath79-usb.c +++ b/drivers/phy/qualcomm/phy-ath79-usb.c @@ -31,7 +31,7 @@ static int ath79_usb_phy_power_on(struct phy *phy) err = reset_control_deassert(priv->reset); if (err && priv->no_suspend_override) - reset_control_assert(priv->no_suspend_override); + reset_control_deassert(priv->no_suspend_override); return err; } -- 2.19.1
[PATCH 8/8] nvmem: core: Avoid useless iterations in nvmem_cell_get_from_lookup()
Once the correct cell has been found there is no need to continue
iterating, just stop there. While at it replace the goto used to leave
the loop with simple break statements.
Signed-off-by: Alban Bedel
---
drivers/nvmem/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 176fe72f4eb5..9334f074defb 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -977,7 +977,7 @@ nvmem_cell_get_from_lookup(struct device *dev, const char
*con_id)
if (IS_ERR(nvmem)) {
/* Provider may not be registered yet. */
cell = ERR_CAST(nvmem);
- goto out;
+ break;
}
cell = nvmem_find_cell_by_name(nvmem,
@@ -985,12 +985,11 @@ nvmem_cell_get_from_lookup(struct device *dev, const char
*con_id)
if (!cell) {
__nvmem_device_put(nvmem);
cell = ERR_PTR(-ENOENT);
- goto out;
}
+ break;
}
}
-out:
mutex_unlock(_lookup_mutex);
return cell;
}
--
2.19.1
[PATCH 3/8] nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional
Add helper functions to make the driver code simpler when a cell is
optional. Using these functions just return NULL when the cell doesn't
exists or if nvmem is disabled.
Signed-off-by: Alban Bedel
---
drivers/nvmem/core.c | 48 ++
include/linux/nvmem-consumer.h | 16
2 files changed, 64 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index f8c43da6f2ca..8e1b52559467 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1083,6 +1083,30 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev,
const char *id)
}
EXPORT_SYMBOL_GPL(nvmem_cell_get);
+/**
+ * nvmem_cell_get_optional() - Get an optional nvmem cell of device from
+ * a given id.
+ *
+ * @dev: Device that requests the nvmem cell.
+ * @cell_id: nvmem cell name to get.
+ *
+ * Return: Will be NULL if no cell with the given name is defined,
+ * an ERR_PTR() on error or a valid pointer to a struct nvmem_cell.
+ * The nvmem_cell will be freed by the nvmem_cell_put().
+ */
+struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
+ const char *cell_id)
+{
+ struct nvmem_cell *cell;
+
+ cell = nvmem_cell_get(dev, cell_id);
+ if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
+ return NULL;
+
+ return cell;
+}
+EXPORT_SYMBOL_GPL(nvmem_cell_get_optional);
+
static void devm_nvmem_cell_release(struct device *dev, void *res)
{
nvmem_cell_put(*(struct nvmem_cell **)res);
@@ -1118,6 +1142,30 @@ struct nvmem_cell *devm_nvmem_cell_get(struct device
*dev, const char *id)
}
EXPORT_SYMBOL_GPL(devm_nvmem_cell_get);
+/**
+ * devm_nvmem_cell_get() - Get an optional nvmem cell of device from
+ * a given id.
+ *
+ * @dev: Device that requests the nvmem cell.
+ * @id: nvmem cell name id to get.
+ *
+ * Return: Will be NULL if the cell doesn't exists, an ERR_PTR() on
+ * error or a valid pointer to a struct nvmem_cell. The nvmem_cell
+ * will be freed by the automatically once the device is freed.
+ */
+struct nvmem_cell *devm_nvmem_cell_get_optional(struct device *dev,
+ const char *cell_id)
+{
+ struct nvmem_cell *cell;
+
+ cell = devm_nvmem_cell_get(dev, cell_id);
+ if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
+ return NULL;
+
+ return cell;
+}
+EXPORT_SYMBOL_GPL(devm_nvmem_cell_get_optional);
+
static int devm_nvmem_cell_match(struct device *dev, void *res, void *data)
{
struct nvmem_cell **c = res;
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 312bfa5efd80..8d7bf21a9adc 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -56,7 +56,11 @@ enum {
/* Cell based interface */
struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id);
+struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
+ const char *id);
struct nvmem_cell *devm_nvmem_cell_get(struct device *dev, const char *id);
+struct nvmem_cell *devm_nvmem_cell_get_optional(struct device *dev,
+ const char *id);
void nvmem_cell_put(struct nvmem_cell *cell);
void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
@@ -96,12 +100,24 @@ static inline struct nvmem_cell *nvmem_cell_get(struct
device *dev,
return ERR_PTR(-EOPNOTSUPP);
}
+static inline struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
+const char *id)
+{
+ return NULL;
+}
+
static inline struct nvmem_cell *devm_nvmem_cell_get(struct device *dev,
const char *id)
{
return ERR_PTR(-EOPNOTSUPP);
}
+static inline struct nvmem_cell *
+devm_nvmem_cell_get_optional(struct device *dev, const char *id)
+{
+ return NULL;
+}
+
static inline void devm_nvmem_cell_put(struct device *dev,
struct nvmem_cell *cell)
{
--
2.19.1
[PATCH 7/8] nvmem: core: Fix device reference leak
__nvmem_device_get() make use of bus_find_device() to get the relevant
device and this function increase the reference count of the device
found, however this is not accounted for anywhere. Fix
__nvmem_device_get() and __nvmem_device_put() to properly release this
reference count.
Signed-off-by: Alban Bedel
---
drivers/nvmem/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 2fa97b373601..176fe72f4eb5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -811,6 +811,7 @@ static struct nvmem_device *__nvmem_device_get(struct
device_node *np,
"could not increase module refcount for cell %s\n",
nvmem_dev_name(nvmem));
+ put_device(>dev);
return ERR_PTR(-EINVAL);
}
@@ -821,6 +822,7 @@ static struct nvmem_device *__nvmem_device_get(struct
device_node *np,
static void __nvmem_device_put(struct nvmem_device *nvmem)
{
+ put_device(>dev);
module_put(nvmem->owner);
kref_put(>refcnt, nvmem_device_release);
}
--
2.19.1
[PATCH 6/8] nvmem: core: Always reference the device returned by nvmem_device_get()
In nvmem_device_get(), when the device lookup fails with DT it currently fallback on nvmem_find() which is wrong for two reasons. First nvmem_find() return NULL when nothing is found instead of an ERR_PTR. But nvmem_find() also just lookup the device, it doesn't reference the module and increment the reference count like it is done in the DT path. To fix this we replace the call to nvmem_find() with a call to __nvmem_device_get() which does all the referencing and return a proper ERR_PTR in case of error. Signed-off-by: Alban Bedel --- drivers/nvmem/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 28e01a9876c6..2fa97b373601 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -874,7 +874,7 @@ struct nvmem_device *nvmem_device_get(struct device *dev, const char *dev_name) } - return nvmem_find(dev_name); + return __nvmem_device_get(NULL, dev_name); } EXPORT_SYMBOL_GPL(nvmem_device_get); -- 2.19.1
[PATCH 4/8] nvmem: core: Fix cell lookup when no cell is found
If the cell list is not empty and nvmem_find_cell_by_node/name() is
called for a cell that is not present in the list they will return an
invalid pointer instead of NULL. This happen because
list_for_each_entry() stop once it reach the list head again, but as
the list head is not contained in a struct nvmem_cell the iteration
variable then contains an invalid value.
This is easily solved by using a variable to iterate over the list and
one to return the cell found.
Signed-off-by: Alban Bedel
---
drivers/nvmem/core.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 8e1b52559467..a7556b20cff4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -525,12 +525,14 @@ static int nvmem_add_cells_from_table(struct nvmem_device
*nvmem)
static struct nvmem_cell *
nvmem_find_cell_by_name(struct nvmem_device *nvmem, const char *cell_id)
{
- struct nvmem_cell *cell = NULL;
+ struct nvmem_cell *iter, *cell = NULL;
mutex_lock(_mutex);
- list_for_each_entry(cell, >cells, node) {
- if (strcmp(cell_id, cell->name) == 0)
+ list_for_each_entry(iter, >cells, node) {
+ if (strcmp(cell_id, iter->name) == 0) {
+ cell = iter;
break;
+ }
}
mutex_unlock(_mutex);
@@ -994,12 +996,14 @@ nvmem_cell_get_from_lookup(struct device *dev, const char
*con_id)
static struct nvmem_cell *
nvmem_find_cell_by_node(struct nvmem_device *nvmem, struct device_node *np)
{
- struct nvmem_cell *cell = NULL;
+ struct nvmem_cell *iter, *cell = NULL;
mutex_lock(_mutex);
- list_for_each_entry(cell, >cells, node) {
- if (np == cell->np)
+ list_for_each_entry(iter, >cells, node) {
+ if (np == iter->np) {
+ cell = iter;
break;
+ }
}
mutex_unlock(_mutex);
--
2.19.1
[PATCH 5/8] nvmem: core: Properly handle connection ID in of_nvmem_device_get()
of_nvmem_device_get() would crash if NULL was passed as a connection
ID. Rework this to use the usual sementic of assuming the first
connection when no connection ID is given.
Furthermore of_nvmem_device_get() would return -EINVAL when it failed
to resolve the connection, making it impossible to properly implement
an optional connection. Return -ENOENT instead to let the caller know
that the connection doesn't exists.
Signed-off-by: Alban Bedel
---
drivers/nvmem/core.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index a7556b20cff4..28e01a9876c6 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -839,13 +839,14 @@ struct nvmem_device *of_nvmem_device_get(struct
device_node *np, const char *id)
{
struct device_node *nvmem_np;
- int index;
+ int index = 0;
- index = of_property_match_string(np, "nvmem-names", id);
+ if (id)
+ index = of_property_match_string(np, "nvmem-names", id);
nvmem_np = of_parse_phandle(np, "nvmem", index);
if (!nvmem_np)
- return ERR_PTR(-EINVAL);
+ return ERR_PTR(-ENOENT);
return __nvmem_device_get(nvmem_np, NULL);
}
--
2.19.1
[PATCH 2/8] nvmem: core: Fix of_nvmem_cell_get() for optional cells
of_nvmem_cell_get() should return -ENOENT when a cell isn't defined, otherwise callers can't distinguish between a missing cell and other errors. Signed-off-by: Alban Bedel --- drivers/nvmem/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index cf2e1091fe89..f8c43da6f2ca 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -1031,7 +1031,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id) cell_np = of_parse_phandle(np, "nvmem-cells", index); if (!cell_np) - return ERR_PTR(-EINVAL); + return ERR_PTR(-ENOENT); nvmem_np = of_get_next_parent(cell_np); if (!nvmem_np) -- 2.19.1
[PATCH 0/8] nvmem: Various small fixes and improvements
Hi, this series is mostly small bug fixes, but also add a new API to make things simpler in drivers that need to request an optional cell. Alban Bedel (8): nvmem: core: Set the provider read-only when no write callback is given nvmem: core: Fix of_nvmem_cell_get() for optional cells nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional nvmem: core: Fix cell lookup when no cell is found nvmem: core: Properly handle connection ID in of_nvmem_device_get() nvmem: core: Always reference the device returned by nvmem_device_get() nvmem: core: Fix device reference leak nvmem: core: Avoid useless iterations in nvmem_cell_get_from_lookup() drivers/nvmem/core.c | 86 +++--- include/linux/nvmem-consumer.h | 16 +++ 2 files changed, 86 insertions(+), 16 deletions(-) -- 2.19.1
[PATCH 1/8] nvmem: core: Set the provider read-only when no write callback is given
If no write callback is given the device should be marked as read-only. While at it also move from a bit or to a logical or as that is a logical expression. Signed-off-by: Alban Bedel --- drivers/nvmem/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index f7301bb4ef3b..cf2e1091fe89 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -646,8 +646,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) config->name ? config->id : nvmem->id); } - nvmem->read_only = device_property_present(config->dev, "read-only") | - config->read_only; + nvmem->read_only = device_property_present(config->dev, "read-only") || + config->read_only || !nvmem->reg_write; if (config->root_only) nvmem->dev.groups = nvmem->read_only ? -- 2.19.1
Re: [PATCH 0/3] namei: implement various scoping AT_* flags
On Sat, Sep 29, 2018 at 12:35 PM Aleksa Sarai wrote: > > The need for some sort of control over VFS's path resolution (to avoid > malicious paths resulting in inadvertent breakouts) has been a very > long-standing desire of many userspace applications. This patchset is a > revival of Al Viro's old AT_NO_JUMPS[1] patchset with a few additions. > > The most obvious change is that AT_NO_JUMPS has been split as dicussed > in the original thread, along with a further split of AT_NO_PROCLINKS > which means that each individual property of AT_NO_JUMPS is now a > separate flag: > > * Path-based escapes from the starting-point using "/" or ".." are > blocked by AT_BENEATH. > * Mountpoint crossings are blocked by AT_XDEV. > * /proc/$pid/fd/$fd resolution is blocked by AT_NO_PROCLINKS (more > correctly it actually blocks any user of nd_jump_link() because it > allows out-of-VFS path resolution manipulation). > > AT_NO_JUMPS is now effectively (AT_BENEATH|AT_XDEV|AT_NO_PROCLINKS). At > Linus' suggestion in the original thread, I've also implemented > AT_NO_SYMLINKS which just denies _all_ symlink resolution (including > "proclink" resolution). It seems quite useful to me. > An additional improvement was made to AT_XDEV. The original AT_NO_JUMPS > path didn't consider "/tmp/.." as a mountpoint crossing -- this patch > blocks this as well (feel free to ask me to remove it if you feel this > is not sane). > > Currently I've only enabled these for openat(2) and the stat(2) family. > I would hope we could enable it for basically every *at(2) syscall -- > but many of them appear to not have a @flags argument and thus we'll > need to add several new syscalls to do this. I'm more than happy to send > those patches, but I'd prefer to know that this preliminary work is > acceptable before doing a bunch of copy-paste to add new sets of *at(2) > syscalls. What do you think of an equivalent feature AT_NO_SYMLINKS flag for mount()? I guess that would have made the fix for CVE-2017-1002101 in Kubernetes easier to write: https://kubernetes.io/blog/2018/04/04/fixing-subpath-volume-vulnerability/ > One additional feature I've implemented is AT_THIS_ROOT (I imagine this > is probably going to be more contentious than the refresh of > AT_NO_JUMPS, so I've included it in a separate patch). The patch itself > describes my reasoning, but the shortened version of the premise is that > continer runtimes need to have a way to resolve paths within a > potentially malicious rootfs. Container runtimes currently do this in > userspace[2] which has implicit race conditions that are not resolvable > in userspace (or use fork+exec+chroot and SCM_RIGHTS passing which is > inefficient). AT_THIS_ROOT allows for per-call chroot-like semantics for > path resolution, which would be invaluable for us -- and the > implementation is basically identical to AT_BENEATH (except that we > don't return errors when someone actually hits the root). > > I've added some selftests for this, but it's not clear to me whether > they should live here or in xfstests (as far as I can tell there are no > other VFS tests in selftests, while there are some tests that look like > generic VFS tests in xfstests). If you'd prefer them to be included in > xfstests, let me know. > > [1]: https://lore.kernel.org/patchwork/patch/784221/ > [2]: https://github.com/cyphar/filepath-securejoin > > Aleksa Sarai (3): > namei: implement O_BENEATH-style AT_* flags > namei: implement AT_THIS_ROOT chroot-like path resolution > selftests: vfs: add AT_* path resolution tests > > fs/fcntl.c| 2 +- > fs/namei.c| 158 -- > fs/open.c | 10 ++ > fs/stat.c | 15 +- > include/linux/fcntl.h | 3 +- > include/linux/namei.h | 8 + > include/uapi/asm-generic/fcntl.h | 20 +++ > include/uapi/linux/fcntl.h| 10 ++ > tools/testing/selftests/Makefile | 1 + > tools/testing/selftests/vfs/.gitignore| 1 + > tools/testing/selftests/vfs/Makefile | 13 ++ > tools/testing/selftests/vfs/at_flags.h| 40 + > tools/testing/selftests/vfs/common.sh | 37 > .../selftests/vfs/tests/0001_at_beneath.sh| 72 > .../selftests/vfs/tests/0002_at_xdev.sh | 54 ++ > .../vfs/tests/0003_at_no_proclinks.sh | 50 ++ > .../vfs/tests/0004_at_no_symlinks.sh | 49 ++ > .../selftests/vfs/tests/0005_at_this_root.sh | 66 > tools/testing/selftests/vfs/vfs_helper.c | 154 + > 19 files changed, 707 insertions(+), 56 deletions(-) > create mode 100644 tools/testing/selftests/vfs/.gitignore > create mode 100644 tools/testing/selftests/vfs/Makefile > create mode 100644 tools/testing/selftests/vfs/at_flags.h > create mode
Re: [PATCH 0/3] namei: implement various scoping AT_* flags
On Sat, Sep 29, 2018 at 12:35 PM Aleksa Sarai wrote: > > The need for some sort of control over VFS's path resolution (to avoid > malicious paths resulting in inadvertent breakouts) has been a very > long-standing desire of many userspace applications. This patchset is a > revival of Al Viro's old AT_NO_JUMPS[1] patchset with a few additions. > > The most obvious change is that AT_NO_JUMPS has been split as dicussed > in the original thread, along with a further split of AT_NO_PROCLINKS > which means that each individual property of AT_NO_JUMPS is now a > separate flag: > > * Path-based escapes from the starting-point using "/" or ".." are > blocked by AT_BENEATH. > * Mountpoint crossings are blocked by AT_XDEV. > * /proc/$pid/fd/$fd resolution is blocked by AT_NO_PROCLINKS (more > correctly it actually blocks any user of nd_jump_link() because it > allows out-of-VFS path resolution manipulation). > > AT_NO_JUMPS is now effectively (AT_BENEATH|AT_XDEV|AT_NO_PROCLINKS). At > Linus' suggestion in the original thread, I've also implemented > AT_NO_SYMLINKS which just denies _all_ symlink resolution (including > "proclink" resolution). It seems quite useful to me. > An additional improvement was made to AT_XDEV. The original AT_NO_JUMPS > path didn't consider "/tmp/.." as a mountpoint crossing -- this patch > blocks this as well (feel free to ask me to remove it if you feel this > is not sane). > > Currently I've only enabled these for openat(2) and the stat(2) family. > I would hope we could enable it for basically every *at(2) syscall -- > but many of them appear to not have a @flags argument and thus we'll > need to add several new syscalls to do this. I'm more than happy to send > those patches, but I'd prefer to know that this preliminary work is > acceptable before doing a bunch of copy-paste to add new sets of *at(2) > syscalls. What do you think of an equivalent feature AT_NO_SYMLINKS flag for mount()? I guess that would have made the fix for CVE-2017-1002101 in Kubernetes easier to write: https://kubernetes.io/blog/2018/04/04/fixing-subpath-volume-vulnerability/ > One additional feature I've implemented is AT_THIS_ROOT (I imagine this > is probably going to be more contentious than the refresh of > AT_NO_JUMPS, so I've included it in a separate patch). The patch itself > describes my reasoning, but the shortened version of the premise is that > continer runtimes need to have a way to resolve paths within a > potentially malicious rootfs. Container runtimes currently do this in > userspace[2] which has implicit race conditions that are not resolvable > in userspace (or use fork+exec+chroot and SCM_RIGHTS passing which is > inefficient). AT_THIS_ROOT allows for per-call chroot-like semantics for > path resolution, which would be invaluable for us -- and the > implementation is basically identical to AT_BENEATH (except that we > don't return errors when someone actually hits the root). > > I've added some selftests for this, but it's not clear to me whether > they should live here or in xfstests (as far as I can tell there are no > other VFS tests in selftests, while there are some tests that look like > generic VFS tests in xfstests). If you'd prefer them to be included in > xfstests, let me know. > > [1]: https://lore.kernel.org/patchwork/patch/784221/ > [2]: https://github.com/cyphar/filepath-securejoin > > Aleksa Sarai (3): > namei: implement O_BENEATH-style AT_* flags > namei: implement AT_THIS_ROOT chroot-like path resolution > selftests: vfs: add AT_* path resolution tests > > fs/fcntl.c| 2 +- > fs/namei.c| 158 -- > fs/open.c | 10 ++ > fs/stat.c | 15 +- > include/linux/fcntl.h | 3 +- > include/linux/namei.h | 8 + > include/uapi/asm-generic/fcntl.h | 20 +++ > include/uapi/linux/fcntl.h| 10 ++ > tools/testing/selftests/Makefile | 1 + > tools/testing/selftests/vfs/.gitignore| 1 + > tools/testing/selftests/vfs/Makefile | 13 ++ > tools/testing/selftests/vfs/at_flags.h| 40 + > tools/testing/selftests/vfs/common.sh | 37 > .../selftests/vfs/tests/0001_at_beneath.sh| 72 > .../selftests/vfs/tests/0002_at_xdev.sh | 54 ++ > .../vfs/tests/0003_at_no_proclinks.sh | 50 ++ > .../vfs/tests/0004_at_no_symlinks.sh | 49 ++ > .../selftests/vfs/tests/0005_at_this_root.sh | 66 > tools/testing/selftests/vfs/vfs_helper.c | 154 + > 19 files changed, 707 insertions(+), 56 deletions(-) > create mode 100644 tools/testing/selftests/vfs/.gitignore > create mode 100644 tools/testing/selftests/vfs/Makefile > create mode 100644 tools/testing/selftests/vfs/at_flags.h > create mode
Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API
On Tue, 21 Aug 2018 14:57:25 +0200 Boris Brezillon wrote: > On Tue, 21 Aug 2018 14:27:16 +0200 > Alban wrote: > > > On Tue, 21 Aug 2018 07:44:04 +0200 > > Boris Brezillon wrote: > > > > > On Tue, 21 Aug 2018 00:53:27 +0200 > > > Alban wrote: > > > > > > > On Sun, 19 Aug 2018 18:46:09 +0200 > > > > Boris Brezillon wrote: > > > > > > > > > On Sun, 19 Aug 2018 13:31:06 +0200 > > > > > Alban wrote: > > > > > > > > > > > On Fri, 17 Aug 2018 18:27:20 +0200 > > > > > > Boris Brezillon wrote: > > > > > > > > > > > > > Hi Bartosz, > > > > > > > > > > > > > > On Fri, 10 Aug 2018 10:05:03 +0200 > > > > > > > Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > From: Alban Bedel > > > > > > > > > > > > > > > > Allow drivers that use the nvmem API to read data > > > > > > > > stored on MTD devices. For this the mtd devices are > > > > > > > > registered as read-only NVMEM providers. > > > > > > > > > > > > > > > > Signed-off-by: Alban Bedel > > > > > > > > [Bartosz: > > > > > > > > - use the managed variant of nvmem_register(), > > > > > > > > - set the nvmem name] > > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > > > > What happened to the 2 other patches of Alban's series? > > > > > > > I'd really like the DT case to be handled/agreed on in > > > > > > > the same patchset, but IIRC, Alban and Srinivas disagreed > > > > > > > on how this should be represented. I hope this time we'll > > > > > > > come to an agreement, because the MTD <-> NVMEM glue has > > > > > > > been floating around for quite some time... > > > > > > > > > > > > These other patches were to fix what I consider a > > > > > > fundamental flaw in the generic NVMEM bindings, however we > > > > > > couldn't agree on this point. Bartosz later contacted me to > > > > > > take over this series and I suggested to just change the > > > > > > MTD NVMEM binding to use a compatible string on the NVMEM > > > > > > cells as an alternative solution to fix the clash with the > > > > > > old style MTD partition. > > > > > > > > > > > > However all this has no impact on the code needed to add > > > > > > NVMEM support to MTD, so the above patch didn't change at > > > > > > all. > > > > > > > > > > It does have an impact on the supported binding though. > > > > > nvmem->dev.of_node is automatically assigned to > > > > > mtd->dev.of_node, which means people will be able to define > > > > > their NVMEM cells directly under the MTD device and reference > > > > > them from other nodes (even if it's not documented), and as > > > > > you said, it conflict with the old MTD partition bindings. So > > > > > we'd better agree on this binding before merging this > > > > > patch. > > > > > > > > Unless the nvmem cell node has a compatible string, then it > > > > won't be considered as a partition by the MTD code. That is > > > > were the clash is, both bindings allow free named child nodes > > > > without a compatible string. > > > > > > Except the current nvmem cells parsing code does not enforce > > > that, and existing DTs rely on this behavior, so we're screwed. > > > Or are you suggesting to add a new "bool check_cells_compat;" > > > field to nvmem_config? > > > > There is no nvmem cell parsing at the moment. The DT lookup just > > resolve the phandle to the cell node, take the parent node and > > search for the nvmem provider that has this OF node. So extending > > it in case the node has a *new* compatible string would not break > > users of the old binding, none of them has a compatible string. > > But we want to enforce the compat check on MTD devices, otherwise old >
Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API
On Tue, 21 Aug 2018 14:57:25 +0200 Boris Brezillon wrote: > On Tue, 21 Aug 2018 14:27:16 +0200 > Alban wrote: > > > On Tue, 21 Aug 2018 07:44:04 +0200 > > Boris Brezillon wrote: > > > > > On Tue, 21 Aug 2018 00:53:27 +0200 > > > Alban wrote: > > > > > > > On Sun, 19 Aug 2018 18:46:09 +0200 > > > > Boris Brezillon wrote: > > > > > > > > > On Sun, 19 Aug 2018 13:31:06 +0200 > > > > > Alban wrote: > > > > > > > > > > > On Fri, 17 Aug 2018 18:27:20 +0200 > > > > > > Boris Brezillon wrote: > > > > > > > > > > > > > Hi Bartosz, > > > > > > > > > > > > > > On Fri, 10 Aug 2018 10:05:03 +0200 > > > > > > > Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > From: Alban Bedel > > > > > > > > > > > > > > > > Allow drivers that use the nvmem API to read data > > > > > > > > stored on MTD devices. For this the mtd devices are > > > > > > > > registered as read-only NVMEM providers. > > > > > > > > > > > > > > > > Signed-off-by: Alban Bedel > > > > > > > > [Bartosz: > > > > > > > > - use the managed variant of nvmem_register(), > > > > > > > > - set the nvmem name] > > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > > > > What happened to the 2 other patches of Alban's series? > > > > > > > I'd really like the DT case to be handled/agreed on in > > > > > > > the same patchset, but IIRC, Alban and Srinivas disagreed > > > > > > > on how this should be represented. I hope this time we'll > > > > > > > come to an agreement, because the MTD <-> NVMEM glue has > > > > > > > been floating around for quite some time... > > > > > > > > > > > > These other patches were to fix what I consider a > > > > > > fundamental flaw in the generic NVMEM bindings, however we > > > > > > couldn't agree on this point. Bartosz later contacted me to > > > > > > take over this series and I suggested to just change the > > > > > > MTD NVMEM binding to use a compatible string on the NVMEM > > > > > > cells as an alternative solution to fix the clash with the > > > > > > old style MTD partition. > > > > > > > > > > > > However all this has no impact on the code needed to add > > > > > > NVMEM support to MTD, so the above patch didn't change at > > > > > > all. > > > > > > > > > > It does have an impact on the supported binding though. > > > > > nvmem->dev.of_node is automatically assigned to > > > > > mtd->dev.of_node, which means people will be able to define > > > > > their NVMEM cells directly under the MTD device and reference > > > > > them from other nodes (even if it's not documented), and as > > > > > you said, it conflict with the old MTD partition bindings. So > > > > > we'd better agree on this binding before merging this > > > > > patch. > > > > > > > > Unless the nvmem cell node has a compatible string, then it > > > > won't be considered as a partition by the MTD code. That is > > > > were the clash is, both bindings allow free named child nodes > > > > without a compatible string. > > > > > > Except the current nvmem cells parsing code does not enforce > > > that, and existing DTs rely on this behavior, so we're screwed. > > > Or are you suggesting to add a new "bool check_cells_compat;" > > > field to nvmem_config? > > > > There is no nvmem cell parsing at the moment. The DT lookup just > > resolve the phandle to the cell node, take the parent node and > > search for the nvmem provider that has this OF node. So extending > > it in case the node has a *new* compatible string would not break > > users of the old binding, none of them has a compatible string. > > But we want to enforce the compat check on MTD devices, otherwise old >
Re: [PATCH linux-next] gpio: fix meaningless return expression
On Tue, 24 Jul 2018 19:57:43 +0800 zhong jiang wrote: > Fix the following sparse error: > > drivers/gpio/gpio-ath79.c:54:16: error: return expression in void > function > > Signed-off-by: zhong jiang Acked-by: Alban Bedel pgpyZ5jQ4E_lO.pgp Description: OpenPGP digital signature
Re: [PATCH linux-next] gpio: fix meaningless return expression
On Tue, 24 Jul 2018 19:57:43 +0800 zhong jiang wrote: > Fix the following sparse error: > > drivers/gpio/gpio-ath79.c:54:16: error: return expression in void > function > > Signed-off-by: zhong jiang Acked-by: Alban Bedel pgpyZ5jQ4E_lO.pgp Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Sun, 10 Jun 2018 11:32:36 +0100
Srinivas Kandagatla wrote:
> On 08/06/18 18:07, Alban wrote:
> > On Fri, 8 Jun 2018 12:34:12 +0100
> > Srinivas Kandagatla wrote:
> >
> ...
> >
> > I looked into this. It would work fine for the cells but not so nicely
> > for the nvmem device API. The phandle for the nvmem device would have
> > to reference the node passed here and not the real device. We would end
> > up with a DT like this:
> >
> > flash@0 {
> > compatible = "mtd";
> > ...
> > nvmem_dev: nvmem-cells {
> > compatible = "nvmem-cells";
> > ...
> > };
> > };
> >
> > other-device@10 {
> > ...
> > nvmem = <_dev>;
> > };
> >
> > Now if there is no cell defined we have this empty child node that make
> > very little sense, it is just there to accommodate the nvmem API.
> >
> NO. This just looks fine!
> nvmem-cells is the nvmem provider node without which you would not have
> any provider instance.
> All this looks as expected!
> Am not sure what is the problem here!
The problem is that DT should represent the hardware, not the OS API.
What should be represented is that other drivers can access data stored
on this device. It is my understanding that this wouldn't be an
acceptable binding as the nvmem provider node would only exists because
of how the NVMEM API currently works, a correct binding would just
directly reference the storage device without this extra node.
> > What I would suggest now is to just change the wording. We don't
> > deprecate the current binding, but we extend it to allow grouping the
> > cells in a child node if required. The code to support this is trivial,
> > (4 lines including error handling) so even if we expect very few
> > bindings to make use of it it is not going to be maintenance drag.
> > That would look like this:
>
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > index fd06c09..085d042 100644
> > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > @@ -19,7 +19,10 @@ Optional properties:
> >
> > = Data cells =
> > These are the child nodes of the provider which contain data cell
> > -information like offset and size in nvmem provider.
> > +information like offset and size in nvmem provider. Alternatively the data
> > +cells can be grouped in a node that has a compatible property set to
> > +"nvmem-cells".
> > +
> >
> > Required properties:
> > reg: specifies the offset in byte within the storage device.
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 4e94a78..3e1369c 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> > device_node *np,
> > if (!nvmem_np)
> > return ERR_PTR(-EINVAL);
> >
> > + /* bindings that already have anonymous child nodes can instead put
> > +* their cells in a child node with an nvmem-cells compatible. */
> > + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
> > + nvmem_np = of_get_next_parent(nvmem_np);
> > + if (!nvmem_np)
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
> > of_node_put(nvmem_np);
> > if (IS_ERR(nvmem))
> >
> > What about it?
> Let me repeat what I have said in my previous emails:
>
> Having a subnode still sounds very fragile to me,
> and this could be much specific case of MTD provider. We might have
> instances where this could be sub-sub node of the the original provider
> for other providers. Also I do not want to bring in Provider specifics
> layout into nvmem bindings.
>
> I can not make myself any clearer than this, Its going to be a NAK from
> my side for the above reasons!
I fully understand you concern but I think they are overblown. First I
highly doubt that more layouts will ever be needed, using a compatible
string pretty much guarantee that we won't clash with another binding.
Furthermore even if you consider this extension "MTD specific" the
amount of code is very small, non intrusive and only run once at
registration time. I would understand if we were talking about pages of
code nesting in various place, but not really when it is a single
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Sun, 10 Jun 2018 11:32:36 +0100
Srinivas Kandagatla wrote:
> On 08/06/18 18:07, Alban wrote:
> > On Fri, 8 Jun 2018 12:34:12 +0100
> > Srinivas Kandagatla wrote:
> >
> ...
> >
> > I looked into this. It would work fine for the cells but not so nicely
> > for the nvmem device API. The phandle for the nvmem device would have
> > to reference the node passed here and not the real device. We would end
> > up with a DT like this:
> >
> > flash@0 {
> > compatible = "mtd";
> > ...
> > nvmem_dev: nvmem-cells {
> > compatible = "nvmem-cells";
> > ...
> > };
> > };
> >
> > other-device@10 {
> > ...
> > nvmem = <_dev>;
> > };
> >
> > Now if there is no cell defined we have this empty child node that make
> > very little sense, it is just there to accommodate the nvmem API.
> >
> NO. This just looks fine!
> nvmem-cells is the nvmem provider node without which you would not have
> any provider instance.
> All this looks as expected!
> Am not sure what is the problem here!
The problem is that DT should represent the hardware, not the OS API.
What should be represented is that other drivers can access data stored
on this device. It is my understanding that this wouldn't be an
acceptable binding as the nvmem provider node would only exists because
of how the NVMEM API currently works, a correct binding would just
directly reference the storage device without this extra node.
> > What I would suggest now is to just change the wording. We don't
> > deprecate the current binding, but we extend it to allow grouping the
> > cells in a child node if required. The code to support this is trivial,
> > (4 lines including error handling) so even if we expect very few
> > bindings to make use of it it is not going to be maintenance drag.
> > That would look like this:
>
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > index fd06c09..085d042 100644
> > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > @@ -19,7 +19,10 @@ Optional properties:
> >
> > = Data cells =
> > These are the child nodes of the provider which contain data cell
> > -information like offset and size in nvmem provider.
> > +information like offset and size in nvmem provider. Alternatively the data
> > +cells can be grouped in a node that has a compatible property set to
> > +"nvmem-cells".
> > +
> >
> > Required properties:
> > reg: specifies the offset in byte within the storage device.
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 4e94a78..3e1369c 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> > device_node *np,
> > if (!nvmem_np)
> > return ERR_PTR(-EINVAL);
> >
> > + /* bindings that already have anonymous child nodes can instead put
> > +* their cells in a child node with an nvmem-cells compatible. */
> > + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
> > + nvmem_np = of_get_next_parent(nvmem_np);
> > + if (!nvmem_np)
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
> > of_node_put(nvmem_np);
> > if (IS_ERR(nvmem))
> >
> > What about it?
> Let me repeat what I have said in my previous emails:
>
> Having a subnode still sounds very fragile to me,
> and this could be much specific case of MTD provider. We might have
> instances where this could be sub-sub node of the the original provider
> for other providers. Also I do not want to bring in Provider specifics
> layout into nvmem bindings.
>
> I can not make myself any clearer than this, Its going to be a NAK from
> my side for the above reasons!
I fully understand you concern but I think they are overblown. First I
highly doubt that more layouts will ever be needed, using a compatible
string pretty much guarantee that we won't clash with another binding.
Furthermore even if you consider this extension "MTD specific" the
amount of code is very small, non intrusive and only run once at
registration time. I would understand if we were talking about pages of
code nesting in various place, but not really when it is a single
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Fri, 8 Jun 2018 12:34:12 +0100
Srinivas Kandagatla wrote:
> >> Can you try this with your original subnode proposal:
> >> just pass the subnode node pointer in np of nvmem_config:
> >>
> >> ->cut<
> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >> index b05aa8e81303..c9621632bbfb 100644
> >> --- a/drivers/nvmem/core.c
> >> +++ b/drivers/nvmem/core.c
> >> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
> >> nvmem_config *config)
> >> nvmem->priv = config->priv;
> >> nvmem->reg_read = config->reg_read;
> >> nvmem->reg_write = config->reg_write;
> >> - nvmem->dev.of_node = config->dev->of_node;
> >> +
> >> + if (config->np)
> >> + nvmem->dev.of_node = config->np;
> >> + else
> >> + nvmem->dev.of_node = config->dev->of_node;
> >>
> >> if (config->id == -1 && config->name) {
> >> dev_set_name(>dev, "%s", config->name);
> >> diff --git a/include/linux/nvmem-provider.h
> >> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> >> 100644 --- a/include/linux/nvmem-provider.h
> >> +++ b/include/linux/nvmem-provider.h
> >> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> >> unsigned int offset,nvmem_device_get(
> >> */
> >>struct nvmem_config {
> >> struct device *dev;
> >> + struct device_node *np;
> >> const char *name;
> >> int id;
> >> struct module *owner;
> >>
> >> ->cut<
> >
> > That should work just fine to allow next to any kind of binding.
> > I'll do a new patch using this approach for the code side and leaving
> > the generic binding as it is.
> Sure!!
> This will give more flexibility to other provider drivers!
I looked into this. It would work fine for the cells but not so nicely
for the nvmem device API. The phandle for the nvmem device would have
to reference the node passed here and not the real device. We would end
up with a DT like this:
flash@0 {
compatible = "mtd";
...
nvmem_dev: nvmem-cells {
compatible = "nvmem-cells";
...
};
};
other-device@10 {
...
nvmem = <_dev>;
};
Now if there is no cell defined we have this empty child node that make
very little sense, it is just there to accommodate the nvmem API.
What I would suggest now is to just change the wording. We don't
deprecate the current binding, but we extend it to allow grouping the
cells in a child node if required. The code to support this is trivial,
(4 lines including error handling) so even if we expect very few
bindings to make use of it it is not going to be maintenance drag.
That would look like this:
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt
b/Documentation/devicetree/bindings/nvmem/nvmem.txt
index fd06c09..085d042 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
@@ -19,7 +19,10 @@ Optional properties:
= Data cells =
These are the child nodes of the provider which contain data cell
-information like offset and size in nvmem provider.
+information like offset and size in nvmem provider. Alternatively the data
+cells can be grouped in a node that has a compatible property set to
+"nvmem-cells".
+
Required properties:
reg: specifies the offset in byte within the storage device.
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4e94a78..3e1369c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node
*np,
if (!nvmem_np)
return ERR_PTR(-EINVAL);
+ /* bindings that already have anonymous child nodes can instead put
+* their cells in a child node with an nvmem-cells compatible. */
+ if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
+ nvmem_np = of_get_next_parent(nvmem_np);
+ if (!nvmem_np)
+ return ERR_PTR(-EINVAL);
+ }
+
nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
of_node_put(nvmem_np);
if (IS_ERR(nvmem))
What about it?
Alban
pgpbNxLP76wQA.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Fri, 8 Jun 2018 12:34:12 +0100
Srinivas Kandagatla wrote:
> >> Can you try this with your original subnode proposal:
> >> just pass the subnode node pointer in np of nvmem_config:
> >>
> >> ->cut<
> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >> index b05aa8e81303..c9621632bbfb 100644
> >> --- a/drivers/nvmem/core.c
> >> +++ b/drivers/nvmem/core.c
> >> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
> >> nvmem_config *config)
> >> nvmem->priv = config->priv;
> >> nvmem->reg_read = config->reg_read;
> >> nvmem->reg_write = config->reg_write;
> >> - nvmem->dev.of_node = config->dev->of_node;
> >> +
> >> + if (config->np)
> >> + nvmem->dev.of_node = config->np;
> >> + else
> >> + nvmem->dev.of_node = config->dev->of_node;
> >>
> >> if (config->id == -1 && config->name) {
> >> dev_set_name(>dev, "%s", config->name);
> >> diff --git a/include/linux/nvmem-provider.h
> >> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> >> 100644 --- a/include/linux/nvmem-provider.h
> >> +++ b/include/linux/nvmem-provider.h
> >> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> >> unsigned int offset,nvmem_device_get(
> >> */
> >>struct nvmem_config {
> >> struct device *dev;
> >> + struct device_node *np;
> >> const char *name;
> >> int id;
> >> struct module *owner;
> >>
> >> ->cut<
> >
> > That should work just fine to allow next to any kind of binding.
> > I'll do a new patch using this approach for the code side and leaving
> > the generic binding as it is.
> Sure!!
> This will give more flexibility to other provider drivers!
I looked into this. It would work fine for the cells but not so nicely
for the nvmem device API. The phandle for the nvmem device would have
to reference the node passed here and not the real device. We would end
up with a DT like this:
flash@0 {
compatible = "mtd";
...
nvmem_dev: nvmem-cells {
compatible = "nvmem-cells";
...
};
};
other-device@10 {
...
nvmem = <_dev>;
};
Now if there is no cell defined we have this empty child node that make
very little sense, it is just there to accommodate the nvmem API.
What I would suggest now is to just change the wording. We don't
deprecate the current binding, but we extend it to allow grouping the
cells in a child node if required. The code to support this is trivial,
(4 lines including error handling) so even if we expect very few
bindings to make use of it it is not going to be maintenance drag.
That would look like this:
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt
b/Documentation/devicetree/bindings/nvmem/nvmem.txt
index fd06c09..085d042 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
@@ -19,7 +19,10 @@ Optional properties:
= Data cells =
These are the child nodes of the provider which contain data cell
-information like offset and size in nvmem provider.
+information like offset and size in nvmem provider. Alternatively the data
+cells can be grouped in a node that has a compatible property set to
+"nvmem-cells".
+
Required properties:
reg: specifies the offset in byte within the storage device.
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4e94a78..3e1369c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node
*np,
if (!nvmem_np)
return ERR_PTR(-EINVAL);
+ /* bindings that already have anonymous child nodes can instead put
+* their cells in a child node with an nvmem-cells compatible. */
+ if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
+ nvmem_np = of_get_next_parent(nvmem_np);
+ if (!nvmem_np)
+ return ERR_PTR(-EINVAL);
+ }
+
nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
of_node_put(nvmem_np);
if (IS_ERR(nvmem))
What about it?
Alban
pgpbNxLP76wQA.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Thu, 7 Jun 2018 18:03:16 +0100
Srinivas Kandagatla wrote:
> On 07/06/18 17:41, Alban wrote:
> > AFAIU the only thing that we disagree on now is if the nodes
> > representing the cells should be direct children of the provider
> > or in a dedicated subnode. For the MTD case both solution would solve
> > the binding clash. I would really appreciate if the DT people could
> Am reluctant in changing the nvmem generic bindings for a special case.
Where I think the generic binding is fundamentally flawed, as this
problem will most probably appear again. But do note that my proposal
doesn't require updating the dts using the original binding, both are
still supported. I proposed deprecating the current binding because I
think it is flawed, but we could also "officially" support both style.
> Can you try this with your original subnode proposal:
> just pass the subnode node pointer in np of nvmem_config:
>
> ->cut<
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..c9621632bbfb 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> nvmem->priv = config->priv;
> nvmem->reg_read = config->reg_read;
> nvmem->reg_write = config->reg_write;
> - nvmem->dev.of_node = config->dev->of_node;
> +
> + if (config->np)
> + nvmem->dev.of_node = config->np;
> + else
> + nvmem->dev.of_node = config->dev->of_node;
>
> if (config->id == -1 && config->name) {
> dev_set_name(>dev, "%s", config->name);
> diff --git a/include/linux/nvmem-provider.h
> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> 100644 --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> unsigned int offset,nvmem_device_get(
>*/
> struct nvmem_config {
> struct device *dev;
> + struct device_node *np;
> const char *name;
> int id;
> struct module *owner;
>
> ->cut<
That should work just fine to allow next to any kind of binding.
I'll do a new patch using this approach for the code side and leaving
the generic binding as it is.
Alban
pgpmU1WMhkr_r.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Thu, 7 Jun 2018 18:03:16 +0100
Srinivas Kandagatla wrote:
> On 07/06/18 17:41, Alban wrote:
> > AFAIU the only thing that we disagree on now is if the nodes
> > representing the cells should be direct children of the provider
> > or in a dedicated subnode. For the MTD case both solution would solve
> > the binding clash. I would really appreciate if the DT people could
> Am reluctant in changing the nvmem generic bindings for a special case.
Where I think the generic binding is fundamentally flawed, as this
problem will most probably appear again. But do note that my proposal
doesn't require updating the dts using the original binding, both are
still supported. I proposed deprecating the current binding because I
think it is flawed, but we could also "officially" support both style.
> Can you try this with your original subnode proposal:
> just pass the subnode node pointer in np of nvmem_config:
>
> ->cut<
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..c9621632bbfb 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> nvmem->priv = config->priv;
> nvmem->reg_read = config->reg_read;
> nvmem->reg_write = config->reg_write;
> - nvmem->dev.of_node = config->dev->of_node;
> +
> + if (config->np)
> + nvmem->dev.of_node = config->np;
> + else
> + nvmem->dev.of_node = config->dev->of_node;
>
> if (config->id == -1 && config->name) {
> dev_set_name(>dev, "%s", config->name);
> diff --git a/include/linux/nvmem-provider.h
> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> 100644 --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> unsigned int offset,nvmem_device_get(
>*/
> struct nvmem_config {
> struct device *dev;
> + struct device_node *np;
> const char *name;
> int id;
> struct module *owner;
>
> ->cut<
That should work just fine to allow next to any kind of binding.
I'll do a new patch using this approach for the code side and leaving
the generic binding as it is.
Alban
pgpmU1WMhkr_r.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 1 May 2018 17:49:03 +0100
Srinivas Kandagatla wrote:
> On 18/04/18 14:34, Alban wrote:
> > On Wed, 18 Apr 2018 13:53:56 +0100
> > Srinivas Kandagatla wrote:
> >
> >> On 18/04/18 13:32, Alban wrote:
> >>>> I was also suggesting you to use nvmem-cell subnode, but make it a
> >>>> proper nvmem provider device, rather than reusing its parent device.
> >>>>
> >>>> You would end up some thing like this in dt.
> >>>>
> >>>> flash@0 {
> >>>> #address-cells = <1>;
> >>>> #size-cells = <1>;
> >>>> compatible = "s25sl064a";
> >>>> reg = <0>;
> >>>>
> >>>> nvmem-cells {
> >>>> compatible = "mtd-nvmem";
> >>>> #address-cells = <1>;
> >>>> #size-cells = <1>;
> >>>>
> >>>> calibration: calib@404 {
> >>>> reg = <0x404 0x10>;
> >>>> };
> >>>> };
> >>>> };
> >>> But the root cause is in the nvmem binding, this conflict could exists
> >> No, the root cause is because of passing wrong device instance to nvmem
> >> core. And trying to workaround is the actual issue.
> >
> > The data is stored on the MTD, so the nvmem provider is the MTD device.
> > I don't think it is a good idea to have a virtual device in the DT to
> > accommodate the nvmem API.
> >
> Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem
> provider too.
>
> However, I would like to see this moving forward.
>
> I can think of one possible solution here, which is, adding
> "nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell.
I would definitely use "nvmem-cell", from the binding point of view it
doesn't matter what the underlaying storage is.
> The problem you mentioned regarding #address-cells and #size-cells with
> provider need to be addressed in nvmem core.
>
> Currently nvmem core only support offsets of 32 bits, if you are
> expecting a 64 bit offsets then we should add that as a feature to nvmem
> core.
>
> nvmem core as it is today should work fine with 32 bit offsets for mtd
> cases.
That's not what I meant, 32 bit should be more that enough for now.
What I meant is that if a binding already has children nodes using
unit-address, then we would end up with two different uses of the same
"address space".
> what do you think?
AFAIU the only thing that we disagree on now is if the nodes
representing the cells should be direct children of the provider
or in a dedicated subnode. For the MTD case both solution would solve
the binding clash. I would really appreciate if the DT people could
chip in so that we can settle this and get the MTD support merged.
Alban
pgplLY5Ckv0pf.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 1 May 2018 17:49:03 +0100
Srinivas Kandagatla wrote:
> On 18/04/18 14:34, Alban wrote:
> > On Wed, 18 Apr 2018 13:53:56 +0100
> > Srinivas Kandagatla wrote:
> >
> >> On 18/04/18 13:32, Alban wrote:
> >>>> I was also suggesting you to use nvmem-cell subnode, but make it a
> >>>> proper nvmem provider device, rather than reusing its parent device.
> >>>>
> >>>> You would end up some thing like this in dt.
> >>>>
> >>>> flash@0 {
> >>>> #address-cells = <1>;
> >>>> #size-cells = <1>;
> >>>> compatible = "s25sl064a";
> >>>> reg = <0>;
> >>>>
> >>>> nvmem-cells {
> >>>> compatible = "mtd-nvmem";
> >>>> #address-cells = <1>;
> >>>> #size-cells = <1>;
> >>>>
> >>>> calibration: calib@404 {
> >>>> reg = <0x404 0x10>;
> >>>> };
> >>>> };
> >>>> };
> >>> But the root cause is in the nvmem binding, this conflict could exists
> >> No, the root cause is because of passing wrong device instance to nvmem
> >> core. And trying to workaround is the actual issue.
> >
> > The data is stored on the MTD, so the nvmem provider is the MTD device.
> > I don't think it is a good idea to have a virtual device in the DT to
> > accommodate the nvmem API.
> >
> Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem
> provider too.
>
> However, I would like to see this moving forward.
>
> I can think of one possible solution here, which is, adding
> "nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell.
I would definitely use "nvmem-cell", from the binding point of view it
doesn't matter what the underlaying storage is.
> The problem you mentioned regarding #address-cells and #size-cells with
> provider need to be addressed in nvmem core.
>
> Currently nvmem core only support offsets of 32 bits, if you are
> expecting a 64 bit offsets then we should add that as a feature to nvmem
> core.
>
> nvmem core as it is today should work fine with 32 bit offsets for mtd
> cases.
That's not what I meant, 32 bit should be more that enough for now.
What I meant is that if a binding already has children nodes using
unit-address, then we would end up with two different uses of the same
"address space".
> what do you think?
AFAIU the only thing that we disagree on now is if the nodes
representing the cells should be direct children of the provider
or in a dedicated subnode. For the MTD case both solution would solve
the binding clash. I would really appreciate if the DT people could
chip in so that we can settle this and get the MTD support merged.
Alban
pgplLY5Ckv0pf.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 4/4] seccomp: add support for passing fds via USER_NOTIF
On Thu, 31 May 2018 at 16:52, Tycho Andersen wrote:
>
> The idea here is that the userspace handler should be able to pass an fd
> back to the trapped task, for example so it can be returned from socket().
>
> I've proposed one API here, but I'm open to other options. In particular,
> this only lets you return an fd from a syscall, which may not be enough in
> all cases. For example, if an fd is written to an output parameter instead
> of returned, the current API can't handle this. Another case is that
> netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
> ever decides to install an fd and output it, we wouldn't be able to handle
> this either.
>
> Still, the vast majority of interesting cases are covered by this API, so
> perhaps it is Enough.
>
> I've left it as a separate commit for two reasons:
> * It illustrates the way in which we would grow struct seccomp_notif and
> struct seccomp_notif_resp without using netlink
> * It shows just how little code is needed to accomplish this :)
>
> v2: new in v2
> v3: no changes
>
> Signed-off-by: Tycho Andersen
> CC: Kees Cook
> CC: Andy Lutomirski
> CC: Oleg Nesterov
> CC: Eric W. Biederman
> CC: "Serge E. Hallyn"
> CC: Christian Brauner
> CC: Tyler Hicks
> CC: Akihiro Suda
> ---
> include/uapi/linux/seccomp.h | 2 +
> kernel/seccomp.c | 49 +++-
> tools/testing/selftests/seccomp/seccomp_bpf.c | 112 ++
> 3 files changed, 161 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 8160e6cad528..3124427219cb 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -71,6 +71,8 @@ struct seccomp_notif_resp {
> __u64 id;
> __s32 error;
> __s64 val;
> + __u8 return_fd;
> + __u32 fd;
> };
>
> #endif /* _UAPI_LINUX_SECCOMP_H */
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 6dc99c65c2f4..2ee958b3efde 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -77,6 +77,8 @@ struct seccomp_knotif {
> /* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
> int error;
> long val;
> + struct file *file;
> + unsigned int flags;
>
> /* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
> struct completion ready;
> @@ -780,10 +782,32 @@ static void seccomp_do_user_notification(int
> this_syscall,
> goto remove_list;
> }
>
> - ret = n.val;
> - err = n.error;
> + if (n.file) {
> + int fd;
> +
> + fd = get_unused_fd_flags(n.flags);
> + if (fd < 0) {
> + err = fd;
> + ret = -1;
> + goto remove_list;
> + }
> +
> + ret = fd;
> + err = 0;
> +
> + fd_install(fd, n.file);
> + /* Don't fput, since fd has a reference now */
> + n.file = NULL;
Do we want the cgroup classid and netprio to be applied here, before
the fd_install? I am looking at similar code in net/core/scm.c
scm_detach_fds():
sock = sock_from_file(fp[i], );
if (sock) {
sock_update_netprioidx(>sk->sk_cgrp_data);
sock_update_classid(>sk->sk_cgrp_data);
}
The listener process might live in a different cgroup with a different
classid & netprio, so it might not be applied as the app might expect.
Also, should we update the struct sock_cgroup_data of the socket, in
order to make the BPF helper function bpf_skb_under_cgroup() work wrt
the cgroup of the target process instead of the listener process? I am
looking at cgroup_sk_alloc(). I don't know what's the correct
behaviour we want here.
> + } else {
> + ret = n.val;
> + err = n.error;
> + }
> +
>
> remove_list:
> + if (n.file)
> + fput(n.file);
> +
> list_del();
> out:
> mutex_unlock(>notify_lock);
> @@ -1598,6 +1622,27 @@ static ssize_t seccomp_notify_write(struct file *file,
> const char __user *buf,
> knotif->state = SECCOMP_NOTIFY_REPLIED;
> knotif->error = resp.error;
> knotif->val = resp.val;
> +
> + if (resp.return_fd) {
> + struct fd fd;
> +
> + /*
> +* This is a little hokey: we need a real fget() (i.e. not
> +* __fget_light(), which is what fdget does), but we also need
> +* the flags from strcut fd. So, we get it, put it, and get it
> +* again for real.
> +*/
> + fd = fdget(resp.fd);
> + knotif->flags = fd.flags;
> + fdput(fd);
> +
> + knotif->file = fget(resp.fd);
> + if (!knotif->file) {
> + ret = -EBADF;
> +
Re: [PATCH v3 4/4] seccomp: add support for passing fds via USER_NOTIF
On Thu, 31 May 2018 at 16:52, Tycho Andersen wrote:
>
> The idea here is that the userspace handler should be able to pass an fd
> back to the trapped task, for example so it can be returned from socket().
>
> I've proposed one API here, but I'm open to other options. In particular,
> this only lets you return an fd from a syscall, which may not be enough in
> all cases. For example, if an fd is written to an output parameter instead
> of returned, the current API can't handle this. Another case is that
> netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
> ever decides to install an fd and output it, we wouldn't be able to handle
> this either.
>
> Still, the vast majority of interesting cases are covered by this API, so
> perhaps it is Enough.
>
> I've left it as a separate commit for two reasons:
> * It illustrates the way in which we would grow struct seccomp_notif and
> struct seccomp_notif_resp without using netlink
> * It shows just how little code is needed to accomplish this :)
>
> v2: new in v2
> v3: no changes
>
> Signed-off-by: Tycho Andersen
> CC: Kees Cook
> CC: Andy Lutomirski
> CC: Oleg Nesterov
> CC: Eric W. Biederman
> CC: "Serge E. Hallyn"
> CC: Christian Brauner
> CC: Tyler Hicks
> CC: Akihiro Suda
> ---
> include/uapi/linux/seccomp.h | 2 +
> kernel/seccomp.c | 49 +++-
> tools/testing/selftests/seccomp/seccomp_bpf.c | 112 ++
> 3 files changed, 161 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 8160e6cad528..3124427219cb 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -71,6 +71,8 @@ struct seccomp_notif_resp {
> __u64 id;
> __s32 error;
> __s64 val;
> + __u8 return_fd;
> + __u32 fd;
> };
>
> #endif /* _UAPI_LINUX_SECCOMP_H */
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 6dc99c65c2f4..2ee958b3efde 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -77,6 +77,8 @@ struct seccomp_knotif {
> /* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
> int error;
> long val;
> + struct file *file;
> + unsigned int flags;
>
> /* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
> struct completion ready;
> @@ -780,10 +782,32 @@ static void seccomp_do_user_notification(int
> this_syscall,
> goto remove_list;
> }
>
> - ret = n.val;
> - err = n.error;
> + if (n.file) {
> + int fd;
> +
> + fd = get_unused_fd_flags(n.flags);
> + if (fd < 0) {
> + err = fd;
> + ret = -1;
> + goto remove_list;
> + }
> +
> + ret = fd;
> + err = 0;
> +
> + fd_install(fd, n.file);
> + /* Don't fput, since fd has a reference now */
> + n.file = NULL;
Do we want the cgroup classid and netprio to be applied here, before
the fd_install? I am looking at similar code in net/core/scm.c
scm_detach_fds():
sock = sock_from_file(fp[i], );
if (sock) {
sock_update_netprioidx(>sk->sk_cgrp_data);
sock_update_classid(>sk->sk_cgrp_data);
}
The listener process might live in a different cgroup with a different
classid & netprio, so it might not be applied as the app might expect.
Also, should we update the struct sock_cgroup_data of the socket, in
order to make the BPF helper function bpf_skb_under_cgroup() work wrt
the cgroup of the target process instead of the listener process? I am
looking at cgroup_sk_alloc(). I don't know what's the correct
behaviour we want here.
> + } else {
> + ret = n.val;
> + err = n.error;
> + }
> +
>
> remove_list:
> + if (n.file)
> + fput(n.file);
> +
> list_del();
> out:
> mutex_unlock(>notify_lock);
> @@ -1598,6 +1622,27 @@ static ssize_t seccomp_notify_write(struct file *file,
> const char __user *buf,
> knotif->state = SECCOMP_NOTIFY_REPLIED;
> knotif->error = resp.error;
> knotif->val = resp.val;
> +
> + if (resp.return_fd) {
> + struct fd fd;
> +
> + /*
> +* This is a little hokey: we need a real fget() (i.e. not
> +* __fget_light(), which is what fdget does), but we also need
> +* the flags from strcut fd. So, we get it, put it, and get it
> +* again for real.
> +*/
> + fd = fdget(resp.fd);
> + knotif->flags = fd.flags;
> + fdput(fd);
> +
> + knotif->file = fget(resp.fd);
> + if (!knotif->file) {
> + ret = -EBADF;
> +
Re: [PATCH] [RFC] bpf: tracing: new helper bpf_get_current_cgroup_ino
id'
> PID TID COMMFUNC -
> 40674067a.out __x64_sys_nanosleep cgid = 106b2
> 40674067 a.out __x64_sys_nanosleep cgid = 106b2
> 40674067a.out __x64_sys_nanosleep cgid = 106b2
> ^C[yhs@localhost tools]$
> The kernel and user space cgid matches. Will provide a
> formal patch later.
> On Mon, May 21, 2018 at 5:24 PM, Y Song <ys114...@gmail.com> wrote:
> > On Mon, May 21, 2018 at 9:26 AM, Alexei Starovoitov
> > <alexei.starovoi...@gmail.com> wrote:
> >> On Sun, May 13, 2018 at 07:33:18PM +0200, Alban Crequy wrote:
> >>>
> >>> +BPF_CALL_2(bpf_get_current_cgroup_ino, u32, hierarchy, u64, flags)
> >>> +{
> >>> + // TODO: pick the correct hierarchy instead of the mem
controller
> >>> + struct cgroup *cgrp = task_cgroup(current, memory_cgrp_id);
> >>> +
> >>> + if (unlikely(!cgrp))
> >>> + return -EINVAL;
> >>> + if (unlikely(hierarchy))
> >>> + return -EINVAL;
> >>> + if (unlikely(flags))
> >>> + return -EINVAL;
> >>> +
> >>> + return cgrp->kn->id.ino;
> >>
> >> ino only is not enough to identify cgroup. It needs generation number
too.
> >> I don't quite see how hierarchy and flags can be used in the future.
> >> Also why limit it to memcg?
> >>
> >> How about something like this instead:
> >>
> >> BPF_CALL_2(bpf_get_current_cgroup_id)
> >> {
> >> struct cgroup *cgrp = task_dfl_cgroup(current);
> >>
> >> return cgrp->kn->id.id;
> >> }
> >> The user space can use fhandle api to get the same 64-bit id.
> >
> > I think this should work. This will also be useful to bcc as user
> > space can encode desired id
> > in the bpf program and compared that id to the current cgroup id, so we
can have
> > cgroup level tracing (esp. stat collection) support. To cope with
> > cgroup hierarchy, user can use
> > cgroup-array based approach or explicitly compare against multiple
cgroup id's.
Re: [PATCH] [RFC] bpf: tracing: new helper bpf_get_current_cgroup_ino
FUNC -
> 40674067a.out __x64_sys_nanosleep cgid = 106b2
> 40674067a.out __x64_sys_nanosleep cgid = 106b2
> 40674067a.out __x64_sys_nanosleep cgid = 106b2
> ^C[yhs@localhost tools]$
> The kernel and user space cgid matches. Will provide a
> formal patch later.
> On Mon, May 21, 2018 at 5:24 PM, Y Song wrote:
> > On Mon, May 21, 2018 at 9:26 AM, Alexei Starovoitov
> > wrote:
> >> On Sun, May 13, 2018 at 07:33:18PM +0200, Alban Crequy wrote:
> >>>
> >>> +BPF_CALL_2(bpf_get_current_cgroup_ino, u32, hierarchy, u64, flags)
> >>> +{
> >>> + // TODO: pick the correct hierarchy instead of the mem
controller
> >>> + struct cgroup *cgrp = task_cgroup(current, memory_cgrp_id);
> >>> +
> >>> + if (unlikely(!cgrp))
> >>> + return -EINVAL;
> >>> + if (unlikely(hierarchy))
> >>> + return -EINVAL;
> >>> + if (unlikely(flags))
> >>> + return -EINVAL;
> >>> +
> >>> + return cgrp->kn->id.ino;
> >>
> >> ino only is not enough to identify cgroup. It needs generation number
too.
> >> I don't quite see how hierarchy and flags can be used in the future.
> >> Also why limit it to memcg?
> >>
> >> How about something like this instead:
> >>
> >> BPF_CALL_2(bpf_get_current_cgroup_id)
> >> {
> >> struct cgroup *cgrp = task_dfl_cgroup(current);
> >>
> >> return cgrp->kn->id.id;
> >> }
> >> The user space can use fhandle api to get the same 64-bit id.
> >
> > I think this should work. This will also be useful to bcc as user
> > space can encode desired id
> > in the bpf program and compared that id to the current cgroup id, so we
can have
> > cgroup level tracing (esp. stat collection) support. To cope with
> > cgroup hierarchy, user can use
> > cgroup-array based approach or explicitly compare against multiple
cgroup id's.
Re: [PATCH] [RFC] bpf: tracing: new helper bpf_get_current_cgroup_ino
On Mon, May 14, 2018 at 9:38 PM, Y Song <ys114...@gmail.com> wrote:
>
> On Sun, May 13, 2018 at 10:33 AM, Alban Crequy <alban.cre...@gmail.com> wrote:
> > From: Alban Crequy <al...@kinvolk.io>
> >
> > bpf_get_current_cgroup_ino() allows BPF trace programs to get the inode
> > of the cgroup where the current process resides.
> >
> > My use case is to get statistics about syscalls done by a specific
> > Kubernetes container. I have a tracepoint on raw_syscalls/sys_enter and
> > a BPF map containing the cgroup inode that I want to trace. I use
> > bpf_get_current_cgroup_ino() and I quickly return from the tracepoint if
> > the inode is not in the BPF hash map.
>
> Alternatively, the kernel already has bpf_current_task_under_cgroup helper
> which uses a cgroup array to store cgroup fd's. If the current task is
> in the hierarchy of a particular cgroup, the helper will return true.
>
> One difference between your helper and bpf_current_task_under_cgroup() is
> that your helper tests against a particular cgroup, not including its
> children, but
> bpf_current_task_under_cgroup() will return true even the task is in a
> nested cgroup.
>
> Maybe this will work for you?
I like the behaviour that it checks for children cgroups. But with the
cgroup array, I can test only one cgroup at a time. I would like to be
able to enable my tracer for a few Kubernetes containers or all by
adding the inodes of a few cgroups in a hash map. So I could keep
separate stats for each. With bpf_current_task_under_cgroup(), I would
need to iterate over the list of cgroups, which is difficult with BPF.
Also, Kubernetes is cgroup-v1 only and bpf_current_task_under_cgroup()
is cgroup-v2 only. In Kubernetes, the processes remain in the root of
the v2 hierarchy. I'd like to be able to select the cgroup hierarchy
in my helper so it'd work for both v1 and v2.
> > Without this BPF helper, I would need to keep track of all pids in the
> > container. The Netlink proc connector can be used to follow process
> > creation and destruction but it is racy.
> >
> > This patch only looks at the memory cgroup, which was enough for me
> > since each Kubernetes container is placed in a different mem cgroup.
> > For a generic implementation, I'm not sure how to proceed: it seems I
> > would need to use 'for_each_root(root)' (see example in
> > proc_cgroup_show() from kernel/cgroup/cgroup.c) but I don't know if
> > taking the cgroup mutex is possible in the BPF helper function. It might
> > be ok in the tracepoint raw_syscalls/sys_enter but could the mutex
> > already be taken in some other tracepoints?
>
> mutex is not allowed in a helper since it can block.
Ok. I don't know how to implement my helper properly then. Maybe I
could just use the 1st cgroup-v1 hierarchy (the name=systemd one) so I
don't have to iterate over the hierarchies. But would that be
acceptable?
Cheers,
Alban
> > Signed-off-by: Alban Crequy <al...@kinvolk.io>
> > ---
> > include/uapi/linux/bpf.h | 11 ++-
> > kernel/trace/bpf_trace.c | 25 +
> > 2 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c5ec89732a8d..38ac3959cdf3 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -755,6 +755,14 @@ union bpf_attr {
> > * @addr: pointer to struct sockaddr to bind socket to
> > * @addr_len: length of sockaddr structure
> > * Return: 0 on success or negative error code
> > + *
> > + * u64 bpf_get_current_cgroup_ino(hierarchy, flags)
> > + * Get the cgroup{1,2} inode of current task under the specified
> > hierarchy.
> > + * @hierarchy: cgroup hierarchy
>
> Not sure what is the value to specify hierarchy here.
> A cgroup directory fd?
>
> > + * @flags: reserved for future use
> > + * Return:
> > + * == 0 error
>
> looks like < 0 means error.
>
> > + *> 0 inode of the cgroup
>>= 0 means good?
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -821,7 +829,8 @@ union bpf_attr {
> > FN(msg_apply_bytes),\
> > FN(msg_cork_bytes), \
> > FN(msg_pull_data), \
> > - FN(bind),
> > + FN(bind), \
> > + FN(get_current_cgroup_ino),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which
> > helper
> > * function eBPF program intends to call
> > diff --git a/kernel/
Re: [PATCH] [RFC] bpf: tracing: new helper bpf_get_current_cgroup_ino
On Mon, May 14, 2018 at 9:38 PM, Y Song wrote:
>
> On Sun, May 13, 2018 at 10:33 AM, Alban Crequy wrote:
> > From: Alban Crequy
> >
> > bpf_get_current_cgroup_ino() allows BPF trace programs to get the inode
> > of the cgroup where the current process resides.
> >
> > My use case is to get statistics about syscalls done by a specific
> > Kubernetes container. I have a tracepoint on raw_syscalls/sys_enter and
> > a BPF map containing the cgroup inode that I want to trace. I use
> > bpf_get_current_cgroup_ino() and I quickly return from the tracepoint if
> > the inode is not in the BPF hash map.
>
> Alternatively, the kernel already has bpf_current_task_under_cgroup helper
> which uses a cgroup array to store cgroup fd's. If the current task is
> in the hierarchy of a particular cgroup, the helper will return true.
>
> One difference between your helper and bpf_current_task_under_cgroup() is
> that your helper tests against a particular cgroup, not including its
> children, but
> bpf_current_task_under_cgroup() will return true even the task is in a
> nested cgroup.
>
> Maybe this will work for you?
I like the behaviour that it checks for children cgroups. But with the
cgroup array, I can test only one cgroup at a time. I would like to be
able to enable my tracer for a few Kubernetes containers or all by
adding the inodes of a few cgroups in a hash map. So I could keep
separate stats for each. With bpf_current_task_under_cgroup(), I would
need to iterate over the list of cgroups, which is difficult with BPF.
Also, Kubernetes is cgroup-v1 only and bpf_current_task_under_cgroup()
is cgroup-v2 only. In Kubernetes, the processes remain in the root of
the v2 hierarchy. I'd like to be able to select the cgroup hierarchy
in my helper so it'd work for both v1 and v2.
> > Without this BPF helper, I would need to keep track of all pids in the
> > container. The Netlink proc connector can be used to follow process
> > creation and destruction but it is racy.
> >
> > This patch only looks at the memory cgroup, which was enough for me
> > since each Kubernetes container is placed in a different mem cgroup.
> > For a generic implementation, I'm not sure how to proceed: it seems I
> > would need to use 'for_each_root(root)' (see example in
> > proc_cgroup_show() from kernel/cgroup/cgroup.c) but I don't know if
> > taking the cgroup mutex is possible in the BPF helper function. It might
> > be ok in the tracepoint raw_syscalls/sys_enter but could the mutex
> > already be taken in some other tracepoints?
>
> mutex is not allowed in a helper since it can block.
Ok. I don't know how to implement my helper properly then. Maybe I
could just use the 1st cgroup-v1 hierarchy (the name=systemd one) so I
don't have to iterate over the hierarchies. But would that be
acceptable?
Cheers,
Alban
> > Signed-off-by: Alban Crequy
> > ---
> > include/uapi/linux/bpf.h | 11 ++-
> > kernel/trace/bpf_trace.c | 25 +
> > 2 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c5ec89732a8d..38ac3959cdf3 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -755,6 +755,14 @@ union bpf_attr {
> > * @addr: pointer to struct sockaddr to bind socket to
> > * @addr_len: length of sockaddr structure
> > * Return: 0 on success or negative error code
> > + *
> > + * u64 bpf_get_current_cgroup_ino(hierarchy, flags)
> > + * Get the cgroup{1,2} inode of current task under the specified
> > hierarchy.
> > + * @hierarchy: cgroup hierarchy
>
> Not sure what is the value to specify hierarchy here.
> A cgroup directory fd?
>
> > + * @flags: reserved for future use
> > + * Return:
> > + * == 0 error
>
> looks like < 0 means error.
>
> > + *> 0 inode of the cgroup
>>= 0 means good?
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -821,7 +829,8 @@ union bpf_attr {
> > FN(msg_apply_bytes),\
> > FN(msg_cork_bytes), \
> > FN(msg_pull_data), \
> > - FN(bind),
> > + FN(bind), \
> > + FN(get_current_cgroup_ino),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which
> > helper
> > * function eBPF program intends to call
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 56ba0f2a01db..9bf92a786639 100644
[PATCH] [RFC] bpf: tracing: new helper bpf_get_current_cgroup_ino
From: Alban Crequy <al...@kinvolk.io>
bpf_get_current_cgroup_ino() allows BPF trace programs to get the inode
of the cgroup where the current process resides.
My use case is to get statistics about syscalls done by a specific
Kubernetes container. I have a tracepoint on raw_syscalls/sys_enter and
a BPF map containing the cgroup inode that I want to trace. I use
bpf_get_current_cgroup_ino() and I quickly return from the tracepoint if
the inode is not in the BPF hash map.
Without this BPF helper, I would need to keep track of all pids in the
container. The Netlink proc connector can be used to follow process
creation and destruction but it is racy.
This patch only looks at the memory cgroup, which was enough for me
since each Kubernetes container is placed in a different mem cgroup.
For a generic implementation, I'm not sure how to proceed: it seems I
would need to use 'for_each_root(root)' (see example in
proc_cgroup_show() from kernel/cgroup/cgroup.c) but I don't know if
taking the cgroup mutex is possible in the BPF helper function. It might
be ok in the tracepoint raw_syscalls/sys_enter but could the mutex
already be taken in some other tracepoints?
Signed-off-by: Alban Crequy <al...@kinvolk.io>
---
include/uapi/linux/bpf.h | 11 ++-
kernel/trace/bpf_trace.c | 25 +
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c5ec89732a8d..38ac3959cdf3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -755,6 +755,14 @@ union bpf_attr {
* @addr: pointer to struct sockaddr to bind socket to
* @addr_len: length of sockaddr structure
* Return: 0 on success or negative error code
+ *
+ * u64 bpf_get_current_cgroup_ino(hierarchy, flags)
+ * Get the cgroup{1,2} inode of current task under the specified hierarchy.
+ * @hierarchy: cgroup hierarchy
+ * @flags: reserved for future use
+ * Return:
+ * == 0 error
+ *> 0 inode of the cgroup
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -821,7 +829,8 @@ union bpf_attr {
FN(msg_apply_bytes),\
FN(msg_cork_bytes), \
FN(msg_pull_data), \
- FN(bind),
+ FN(bind), \
+ FN(get_current_cgroup_ino),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 56ba0f2a01db..9bf92a786639 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -524,6 +524,29 @@ static const struct bpf_func_proto
bpf_probe_read_str_proto = {
.arg3_type = ARG_ANYTHING,
};
+BPF_CALL_2(bpf_get_current_cgroup_ino, u32, hierarchy, u64, flags)
+{
+ // TODO: pick the correct hierarchy instead of the mem controller
+ struct cgroup *cgrp = task_cgroup(current, memory_cgrp_id);
+
+ if (unlikely(!cgrp))
+ return -EINVAL;
+ if (unlikely(hierarchy))
+ return -EINVAL;
+ if (unlikely(flags))
+ return -EINVAL;
+
+ return cgrp->kn->id.ino;
+}
+
+static const struct bpf_func_proto bpf_get_current_cgroup_ino_proto = {
+ .func = bpf_get_current_cgroup_ino,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_DONTCARE,
+ .arg2_type = ARG_DONTCARE,
+};
+
static const struct bpf_func_proto *
tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -564,6 +587,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct
bpf_prog *prog)
return _get_prandom_u32_proto;
case BPF_FUNC_probe_read_str:
return _probe_read_str_proto;
+ case BPF_FUNC_get_current_cgroup_ino:
+ return _get_current_cgroup_ino_proto;
default:
return NULL;
}
--
2.14.3
[PATCH] [RFC] bpf: tracing: new helper bpf_get_current_cgroup_ino
From: Alban Crequy
bpf_get_current_cgroup_ino() allows BPF trace programs to get the inode
of the cgroup where the current process resides.
My use case is to get statistics about syscalls done by a specific
Kubernetes container. I have a tracepoint on raw_syscalls/sys_enter and
a BPF map containing the cgroup inode that I want to trace. I use
bpf_get_current_cgroup_ino() and I quickly return from the tracepoint if
the inode is not in the BPF hash map.
Without this BPF helper, I would need to keep track of all pids in the
container. The Netlink proc connector can be used to follow process
creation and destruction but it is racy.
This patch only looks at the memory cgroup, which was enough for me
since each Kubernetes container is placed in a different mem cgroup.
For a generic implementation, I'm not sure how to proceed: it seems I
would need to use 'for_each_root(root)' (see example in
proc_cgroup_show() from kernel/cgroup/cgroup.c) but I don't know if
taking the cgroup mutex is possible in the BPF helper function. It might
be ok in the tracepoint raw_syscalls/sys_enter but could the mutex
already be taken in some other tracepoints?
Signed-off-by: Alban Crequy
---
include/uapi/linux/bpf.h | 11 ++-
kernel/trace/bpf_trace.c | 25 +
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c5ec89732a8d..38ac3959cdf3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -755,6 +755,14 @@ union bpf_attr {
* @addr: pointer to struct sockaddr to bind socket to
* @addr_len: length of sockaddr structure
* Return: 0 on success or negative error code
+ *
+ * u64 bpf_get_current_cgroup_ino(hierarchy, flags)
+ * Get the cgroup{1,2} inode of current task under the specified hierarchy.
+ * @hierarchy: cgroup hierarchy
+ * @flags: reserved for future use
+ * Return:
+ * == 0 error
+ *> 0 inode of the cgroup
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -821,7 +829,8 @@ union bpf_attr {
FN(msg_apply_bytes),\
FN(msg_cork_bytes), \
FN(msg_pull_data), \
- FN(bind),
+ FN(bind), \
+ FN(get_current_cgroup_ino),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 56ba0f2a01db..9bf92a786639 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -524,6 +524,29 @@ static const struct bpf_func_proto
bpf_probe_read_str_proto = {
.arg3_type = ARG_ANYTHING,
};
+BPF_CALL_2(bpf_get_current_cgroup_ino, u32, hierarchy, u64, flags)
+{
+ // TODO: pick the correct hierarchy instead of the mem controller
+ struct cgroup *cgrp = task_cgroup(current, memory_cgrp_id);
+
+ if (unlikely(!cgrp))
+ return -EINVAL;
+ if (unlikely(hierarchy))
+ return -EINVAL;
+ if (unlikely(flags))
+ return -EINVAL;
+
+ return cgrp->kn->id.ino;
+}
+
+static const struct bpf_func_proto bpf_get_current_cgroup_ino_proto = {
+ .func = bpf_get_current_cgroup_ino,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_DONTCARE,
+ .arg2_type = ARG_DONTCARE,
+};
+
static const struct bpf_func_proto *
tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -564,6 +587,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct
bpf_prog *prog)
return _get_prandom_u32_proto;
case BPF_FUNC_probe_read_str:
return _probe_read_str_proto;
+ case BPF_FUNC_get_current_cgroup_ino:
+ return _get_current_cgroup_ino_proto;
default:
return NULL;
}
--
2.14.3
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Wed, 18 Apr 2018 13:53:56 +0100
Srinivas Kandagatla <srinivas.kandaga...@linaro.org> wrote:
> On 18/04/18 13:32, Alban wrote:
> >> I was also suggesting you to use nvmem-cell subnode, but make it a
> >> proper nvmem provider device, rather than reusing its parent device.
> >>
> >> You would end up some thing like this in dt.
> >>
> >> flash@0 {
> >>#address-cells = <1>;
> >>#size-cells = <1>;
> >>compatible = "s25sl064a";
> >>reg = <0>;
> >>
> >>nvmem-cells {
> >>compatible = "mtd-nvmem";
> >>#address-cells = <1>;
> >>#size-cells = <1>;
> >>
> >>calibration: calib@404 {
> >>reg = <0x404 0x10>;
> >>};
> >>};
> >> };
> > But the root cause is in the nvmem binding, this conflict could exists
> No, the root cause is because of passing wrong device instance to nvmem
> core. And trying to workaround is the actual issue.
The data is stored on the MTD, so the nvmem provider is the MTD device.
I don't think it is a good idea to have a virtual device in the DT to
accommodate the nvmem API.
> > with any device type, not just MTD. I don't understand why we would want
> > such workarounds instead of just fixing the problem once and for all.
> AFAIU, This is not a workaround, this is how nvmem provider bindings are
> and all providers should try to follow it.
>
> I still do not understand what is the issue in making nvmem-cells node a
> proper nvmem provider device?
It is doable, but beside making the code more complex, AFAIU that would
goes against DT best practice as no such device exists in the hardware.
The DT should only represent that this device contains data that can be
requested by a driver, and ideally in the same way for all kind of
storages.
Alban
pgphbDMhrQg7O.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Wed, 18 Apr 2018 13:53:56 +0100
Srinivas Kandagatla wrote:
> On 18/04/18 13:32, Alban wrote:
> >> I was also suggesting you to use nvmem-cell subnode, but make it a
> >> proper nvmem provider device, rather than reusing its parent device.
> >>
> >> You would end up some thing like this in dt.
> >>
> >> flash@0 {
> >>#address-cells = <1>;
> >>#size-cells = <1>;
> >>compatible = "s25sl064a";
> >>reg = <0>;
> >>
> >>nvmem-cells {
> >>compatible = "mtd-nvmem";
> >>#address-cells = <1>;
> >>#size-cells = <1>;
> >>
> >>calibration: calib@404 {
> >>reg = <0x404 0x10>;
> >>};
> >>};
> >> };
> > But the root cause is in the nvmem binding, this conflict could exists
> No, the root cause is because of passing wrong device instance to nvmem
> core. And trying to workaround is the actual issue.
The data is stored on the MTD, so the nvmem provider is the MTD device.
I don't think it is a good idea to have a virtual device in the DT to
accommodate the nvmem API.
> > with any device type, not just MTD. I don't understand why we would want
> > such workarounds instead of just fixing the problem once and for all.
> AFAIU, This is not a workaround, this is how nvmem provider bindings are
> and all providers should try to follow it.
>
> I still do not understand what is the issue in making nvmem-cells node a
> proper nvmem provider device?
It is doable, but beside making the code more complex, AFAIU that would
goes against DT best practice as no such device exists in the hardware.
The DT should only represent that this device contains data that can be
requested by a driver, and ideally in the same way for all kind of
storages.
Alban
pgphbDMhrQg7O.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Wed, 18 Apr 2018 13:12:48 +0100
Srinivas Kandagatla <srinivas.kandaga...@linaro.org> wrote:
> On 18/04/18 12:41, Alban wrote:
> > On Tue, 17 Apr 2018 18:00:40 +0200
> > Alban <al...@free.fr> wrote:
> >
> >> On Tue, 17 Apr 2018 16:44:01 +0100
> >> Srinivas Kandagatla <srinivas.kandaga...@linaro.org> wrote:
> >>
> >>> Thanks for explaining,
> >>>
> >>> On 17/04/18 15:54, Alban wrote:
> >>>> This will not only allow reading the calibration data from nvmem, but
> >>>> will also create a partition on the MTD device, which is not acceptable.
> >>>> With my proposed binding this would become:
> >>>>
> >>>> flash@0 {
> >>>> #address-cells = <1>;
> >>>> #size-cells = <1>;
> >>>> compatible = "s25sl064a";
> >>>> reg = <0>;
> >>>>
> >>>> nvmem-cells {
> >>>> compatible = "nvmem-cells";
> >>>> #address-cells = <1>;
> >>>> #address-cells = <1>;
> >>>>
> >>>> calibration: calib@404 {
> >>>> reg = <0x404 0x10>;
> >>>> };
> >>>> };
> >>>
> >>> Why can't we make nvmem-cells node a nvmem provider in this case?
> >>> Which should work!
> >>
> >> TBH I just copied what have been done to fix the same problem with the
> >> MTD partitions. But yes we could also just extend the current binding
> >> to require a compatible string on each nvmem-cell, which would not
> >> require any code change to support.
> >
> > However this scheme will not work if the device node binding already
> > have subnodes with addresses. The addressing, as specified by
> > #address-cells and #size-cells, might be incompatible or might overlap.
> > Using the nvmem-cells subnode solve this problem.
> >
>
> I was also suggesting you to use nvmem-cell subnode, but make it a
> proper nvmem provider device, rather than reusing its parent device.
>
> You would end up some thing like this in dt.
>
> flash@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "s25sl064a";
> reg = <0>;
>
> nvmem-cells {
> compatible = "mtd-nvmem";
> #address-cells = <1>;
> #size-cells = <1>;
>
> calibration: calib@404 {
> reg = <0x404 0x10>;
> };
> };
> };
But the root cause is in the nvmem binding, this conflict could exists
with any device type, not just MTD. I don't understand why we would want
such workarounds instead of just fixing the problem once and for all.
Alban
pgpUtA8bBqPuh.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Wed, 18 Apr 2018 13:12:48 +0100
Srinivas Kandagatla wrote:
> On 18/04/18 12:41, Alban wrote:
> > On Tue, 17 Apr 2018 18:00:40 +0200
> > Alban wrote:
> >
> >> On Tue, 17 Apr 2018 16:44:01 +0100
> >> Srinivas Kandagatla wrote:
> >>
> >>> Thanks for explaining,
> >>>
> >>> On 17/04/18 15:54, Alban wrote:
> >>>> This will not only allow reading the calibration data from nvmem, but
> >>>> will also create a partition on the MTD device, which is not acceptable.
> >>>> With my proposed binding this would become:
> >>>>
> >>>> flash@0 {
> >>>> #address-cells = <1>;
> >>>> #size-cells = <1>;
> >>>> compatible = "s25sl064a";
> >>>> reg = <0>;
> >>>>
> >>>> nvmem-cells {
> >>>> compatible = "nvmem-cells";
> >>>> #address-cells = <1>;
> >>>> #address-cells = <1>;
> >>>>
> >>>> calibration: calib@404 {
> >>>> reg = <0x404 0x10>;
> >>>> };
> >>>> };
> >>>
> >>> Why can't we make nvmem-cells node a nvmem provider in this case?
> >>> Which should work!
> >>
> >> TBH I just copied what have been done to fix the same problem with the
> >> MTD partitions. But yes we could also just extend the current binding
> >> to require a compatible string on each nvmem-cell, which would not
> >> require any code change to support.
> >
> > However this scheme will not work if the device node binding already
> > have subnodes with addresses. The addressing, as specified by
> > #address-cells and #size-cells, might be incompatible or might overlap.
> > Using the nvmem-cells subnode solve this problem.
> >
>
> I was also suggesting you to use nvmem-cell subnode, but make it a
> proper nvmem provider device, rather than reusing its parent device.
>
> You would end up some thing like this in dt.
>
> flash@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "s25sl064a";
> reg = <0>;
>
> nvmem-cells {
> compatible = "mtd-nvmem";
> #address-cells = <1>;
> #size-cells = <1>;
>
> calibration: calib@404 {
> reg = <0x404 0x10>;
> };
> };
> };
But the root cause is in the nvmem binding, this conflict could exists
with any device type, not just MTD. I don't understand why we would want
such workarounds instead of just fixing the problem once and for all.
Alban
pgpUtA8bBqPuh.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 17 Apr 2018 18:00:40 +0200
Alban <al...@free.fr> wrote:
> On Tue, 17 Apr 2018 16:44:01 +0100
> Srinivas Kandagatla <srinivas.kandaga...@linaro.org> wrote:
>
> > Thanks for explaining,
> >
> > On 17/04/18 15:54, Alban wrote:
> > > This will not only allow reading the calibration data from nvmem, but
> > > will also create a partition on the MTD device, which is not acceptable.
> > > With my proposed binding this would become:
> > >
> > > flash@0 {
> > > #address-cells = <1>;
> > > #size-cells = <1>;
> > > compatible = "s25sl064a";
> > > reg = <0>;
> > >
> > > nvmem-cells {
> > > compatible = "nvmem-cells";
> > > #address-cells = <1>;
> > > #address-cells = <1>;
> > >
> > > calibration: calib@404 {
> > > reg = <0x404 0x10>;
> > > };
> > > };
> >
> > Why can't we make nvmem-cells node a nvmem provider in this case?
> > Which should work!
>
> TBH I just copied what have been done to fix the same problem with the
> MTD partitions. But yes we could also just extend the current binding
> to require a compatible string on each nvmem-cell, which would not
> require any code change to support.
However this scheme will not work if the device node binding already
have subnodes with addresses. The addressing, as specified by
#address-cells and #size-cells, might be incompatible or might overlap.
Using the nvmem-cells subnode solve this problem.
Alban
pgpzPU9Zo869q.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 17 Apr 2018 18:00:40 +0200
Alban wrote:
> On Tue, 17 Apr 2018 16:44:01 +0100
> Srinivas Kandagatla wrote:
>
> > Thanks for explaining,
> >
> > On 17/04/18 15:54, Alban wrote:
> > > This will not only allow reading the calibration data from nvmem, but
> > > will also create a partition on the MTD device, which is not acceptable.
> > > With my proposed binding this would become:
> > >
> > > flash@0 {
> > > #address-cells = <1>;
> > > #size-cells = <1>;
> > > compatible = "s25sl064a";
> > > reg = <0>;
> > >
> > > nvmem-cells {
> > > compatible = "nvmem-cells";
> > > #address-cells = <1>;
> > > #address-cells = <1>;
> > >
> > > calibration: calib@404 {
> > > reg = <0x404 0x10>;
> > > };
> > > };
> >
> > Why can't we make nvmem-cells node a nvmem provider in this case?
> > Which should work!
>
> TBH I just copied what have been done to fix the same problem with the
> MTD partitions. But yes we could also just extend the current binding
> to require a compatible string on each nvmem-cell, which would not
> require any code change to support.
However this scheme will not work if the device node binding already
have subnodes with addresses. The addressing, as specified by
#address-cells and #size-cells, might be incompatible or might overlap.
Using the nvmem-cells subnode solve this problem.
Alban
pgpzPU9Zo869q.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 17 Apr 2018 16:44:01 +0100
Srinivas Kandagatla <srinivas.kandaga...@linaro.org> wrote:
> Thanks for explaining,
>
> On 17/04/18 15:54, Alban wrote:
> > This will not only allow reading the calibration data from nvmem, but
> > will also create a partition on the MTD device, which is not acceptable.
> > With my proposed binding this would become:
> >
> > flash@0 {
> > #address-cells = <1>;
> > #size-cells = <1>;
> > compatible = "s25sl064a";
> > reg = <0>;
> >
> > nvmem-cells {
> > compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #address-cells = <1>;
> >
> > calibration: calib@404 {
> > reg = <0x404 0x10>;
> > };
> > };
>
> Why can't we make nvmem-cells node a nvmem provider in this case?
> Which should work!
TBH I just copied what have been done to fix the same problem with the
MTD partitions. But yes we could also just extend the current binding
to require a compatible string on each nvmem-cell, which would not
require any code change to support.
Alban
pgpnay9Lx5iOE.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 17 Apr 2018 16:44:01 +0100
Srinivas Kandagatla wrote:
> Thanks for explaining,
>
> On 17/04/18 15:54, Alban wrote:
> > This will not only allow reading the calibration data from nvmem, but
> > will also create a partition on the MTD device, which is not acceptable.
> > With my proposed binding this would become:
> >
> > flash@0 {
> > #address-cells = <1>;
> > #size-cells = <1>;
> > compatible = "s25sl064a";
> > reg = <0>;
> >
> > nvmem-cells {
> > compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #address-cells = <1>;
> >
> > calibration: calib@404 {
> > reg = <0x404 0x10>;
> > };
> > };
>
> Why can't we make nvmem-cells node a nvmem provider in this case?
> Which should work!
TBH I just copied what have been done to fix the same problem with the
MTD partitions. But yes we could also just extend the current binding
to require a compatible string on each nvmem-cell, which would not
require any code change to support.
Alban
pgpnay9Lx5iOE.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 17 Apr 2018 13:54:07 +0100
Srinivas Kandagatla <srinivas.kandaga...@linaro.org> wrote:
> On 24/03/18 23:24, Alban Bedel wrote:
> > Having the cells as subnodes of the provider device without any
> > compatible property might clash with other bindings. To avoid this
> > problem update the binding to have all the cells in a 'nvmem-cells'
> > subnode with a 'nvmem-cells' compatible string. This new binding
> > guarantee that we can turn any kind of device in a nvmem provider.
> >
> > While discouraged for new uses the old scheme is still supported for
> > backward compatibility.
>
> Am not sure if this a really good idea to change nvmem bindings based on
> provider requirements. This can be a beginning of other problems!!
I think you misunderstood something here, this proposed new binding
would be for all new nvmem bindings, not just mtd backed nvmem.
> Did you know that we can pass nvmem cells info via nvmem config ?
>
> Why can't mtd-nvmem provider populate the nvmem_config->cells from
> its dt "nvmem-cells" subnode before it registers the provider?
The DT based lookup of nvmem-cells doesn't use nvmem_config->cells, so
that's not an option. In fact here the problem come from the MTD side
because it also had a similar binding using subnodes without compatible
string. Just to make things clear, here is an example of the clash
using the current nvmem binding on an unpartitioned MTD device:
flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "s25sl064a";
reg = <0>;
calibration: calib@404 {
reg = <0x404 0x10>;
};
};
This will not only allow reading the calibration data from nvmem, but
will also create a partition on the MTD device, which is not acceptable.
With my proposed binding this would become:
flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "s25sl064a";
reg = <0>;
nvmem-cells {
compatible = "nvmem-cells";
#address-cells = <1>;
#address-cells = <1>;
calibration: calib@404 {
reg = <0x404 0x10>;
};
};
};
Which would work fine as the MTD code will ignore the nvmem-cells
subnode thanks to its compatible string.
IMHO subnodes without any compatible properties should never be used in
such generic bindings, as it is very likely that it will at some point
clash with another generic binding or with a device specific binding.
Alban
pgpr_H6BgeE9w.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Tue, 17 Apr 2018 13:54:07 +0100
Srinivas Kandagatla wrote:
> On 24/03/18 23:24, Alban Bedel wrote:
> > Having the cells as subnodes of the provider device without any
> > compatible property might clash with other bindings. To avoid this
> > problem update the binding to have all the cells in a 'nvmem-cells'
> > subnode with a 'nvmem-cells' compatible string. This new binding
> > guarantee that we can turn any kind of device in a nvmem provider.
> >
> > While discouraged for new uses the old scheme is still supported for
> > backward compatibility.
>
> Am not sure if this a really good idea to change nvmem bindings based on
> provider requirements. This can be a beginning of other problems!!
I think you misunderstood something here, this proposed new binding
would be for all new nvmem bindings, not just mtd backed nvmem.
> Did you know that we can pass nvmem cells info via nvmem config ?
>
> Why can't mtd-nvmem provider populate the nvmem_config->cells from
> its dt "nvmem-cells" subnode before it registers the provider?
The DT based lookup of nvmem-cells doesn't use nvmem_config->cells, so
that's not an option. In fact here the problem come from the MTD side
because it also had a similar binding using subnodes without compatible
string. Just to make things clear, here is an example of the clash
using the current nvmem binding on an unpartitioned MTD device:
flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "s25sl064a";
reg = <0>;
calibration: calib@404 {
reg = <0x404 0x10>;
};
};
This will not only allow reading the calibration data from nvmem, but
will also create a partition on the MTD device, which is not acceptable.
With my proposed binding this would become:
flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "s25sl064a";
reg = <0>;
nvmem-cells {
compatible = "nvmem-cells";
#address-cells = <1>;
#address-cells = <1>;
calibration: calib@404 {
reg = <0x404 0x10>;
};
};
};
Which would work fine as the MTD code will ignore the nvmem-cells
subnode thanks to its compatible string.
IMHO subnodes without any compatible properties should never be used in
such generic bindings, as it is very likely that it will at some point
clash with another generic binding or with a device specific binding.
Alban
pgpr_H6BgeE9w.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 2/3] doc: bindings: Add bindings documentation for mtd nvmem
On Mon, 16 Apr 2018 16:08:51 -0500
Rob Herring <r...@kernel.org> wrote:
> On Sun, Mar 25, 2018 at 12:24:58AM +0100, Alban Bedel wrote:
> > Config data for drivers, like MAC addresses, is often stored in MTD.
> > Add a binding that define how such data storage can be represented in
> > device tree.
> >
> > Signed-off-by: Alban Bedel <al...@free.fr>
> > ---
> > Changelog:
> > v2: * Added a "Required properties" section with the nvmem-provider
> > property
> > v3: * Fixed my name in From and Signed-off-by
> > * Moved to the new nvmem binding with the nvmem-cells subnode
> > ---
> > .../devicetree/bindings/nvmem/mtd-nvmem.txt| 27
> > ++
> > 1 file changed, 27 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 000..c819a69
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,27 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > +An MTD device, or one of its partition, can be defined as a NVMEM provider
> > +by having an 'nvmem-cells' subnode as defined in nvmem.txt.
> > +
> > +Example:
> > +
> > + flash@0 {
> > + ...
> > +
> > + partition@2 {
>
> This unit address is not correct...
Is it because I ellipsed the #address-cells and #size-cells? I will add
them.
> > + label = "art";
> > + reg = <0x7F 0x01>;
>
> Lowercase hex.
Will do.
> > + read-only;
> > +
> > + nvmem-cells {
> > + compatible = "nvmem-cells";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + eeprom@1000 {
>
> "eeprom" isn't specific data. The purpose of the nvmem binding is to
> provide specific data fields like MAC addresseses.
>
> Plus "eeprom" is the node name for EEPROM devices.
This example is from a board using an ath9k chip and this field contains
what the driver names EEPROM, although it is in fact only a config
binary blob. However I agree that it is misleading for such an example,
I will replace it with something like "wifi-config-data".
Alban
pgpNTiE89dkFl.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 2/3] doc: bindings: Add bindings documentation for mtd nvmem
On Mon, 16 Apr 2018 16:08:51 -0500
Rob Herring wrote:
> On Sun, Mar 25, 2018 at 12:24:58AM +0100, Alban Bedel wrote:
> > Config data for drivers, like MAC addresses, is often stored in MTD.
> > Add a binding that define how such data storage can be represented in
> > device tree.
> >
> > Signed-off-by: Alban Bedel
> > ---
> > Changelog:
> > v2: * Added a "Required properties" section with the nvmem-provider
> > property
> > v3: * Fixed my name in From and Signed-off-by
> > * Moved to the new nvmem binding with the nvmem-cells subnode
> > ---
> > .../devicetree/bindings/nvmem/mtd-nvmem.txt| 27
> > ++
> > 1 file changed, 27 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 000..c819a69
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,27 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > +An MTD device, or one of its partition, can be defined as a NVMEM provider
> > +by having an 'nvmem-cells' subnode as defined in nvmem.txt.
> > +
> > +Example:
> > +
> > + flash@0 {
> > + ...
> > +
> > + partition@2 {
>
> This unit address is not correct...
Is it because I ellipsed the #address-cells and #size-cells? I will add
them.
> > + label = "art";
> > + reg = <0x7F 0x01>;
>
> Lowercase hex.
Will do.
> > + read-only;
> > +
> > + nvmem-cells {
> > + compatible = "nvmem-cells";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + eeprom@1000 {
>
> "eeprom" isn't specific data. The purpose of the nvmem binding is to
> provide specific data fields like MAC addresseses.
>
> Plus "eeprom" is the node name for EEPROM devices.
This example is from a board using an ath9k chip and this field contains
what the driver names EEPROM, although it is in fact only a config
binary blob. However I agree that it is misleading for such an example,
I will replace it with something like "wifi-config-data".
Alban
pgpNTiE89dkFl.pgp
Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Mon, 16 Apr 2018 16:04:29 -0500 Rob Herring <r...@kernel.org> wrote: > On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote: > > Having the cells as subnodes of the provider device without any > > compatible property might clash with other bindings. To avoid this > > problem update the binding to have all the cells in a 'nvmem-cells' > > subnode with a 'nvmem-cells' compatible string. This new binding > > guarantee that we can turn any kind of device in a nvmem provider. > > > > While discouraged for new uses the old scheme is still supported for > > backward compatibility. > > > > Signed-off-by: Alban Bedel <al...@free.fr> > > --- > > Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 > > --- > > drivers/nvmem/core.c | 10 + > > 2 files changed, 48 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt > > b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > index fd06c09..6b723e7 100644 > > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > @@ -11,14 +11,29 @@ these data from, and where they are stored on the > > storage device. > > This document is here to document this. > > > > = Data providers = > > -Contains bindings specific to provider drivers and data cells as children > > -of this node. > > +A data provider should have a subnode named 'nvmem-cells' that contains > > +a subnodes for each data cells. > > + > > +For backward compatibility the nvmem data cells can be direct children > > +of the data provider. This use is discouraged as it can conflict with > > +other bindings. > > I don't think we need to go this far. Whether this is necessary depends > on the provider. It depend more on the drivers implementation. Sure as long as the driver only support the nvmem API it doesn't matter, both binding are fine. But if it ever need to support another API the bindings might clash and the whole device binding will need to be updated. So all in all I see very few value in still allowing the old binding for new devices, or do you seen any problem with the new binding? However if the consensus is to keep both styles I will rewrite this paragraph as needed. Alban pgplORqZJWcEN.pgp Description: OpenPGP digital signature
Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
On Mon, 16 Apr 2018 16:04:29 -0500 Rob Herring wrote: > On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote: > > Having the cells as subnodes of the provider device without any > > compatible property might clash with other bindings. To avoid this > > problem update the binding to have all the cells in a 'nvmem-cells' > > subnode with a 'nvmem-cells' compatible string. This new binding > > guarantee that we can turn any kind of device in a nvmem provider. > > > > While discouraged for new uses the old scheme is still supported for > > backward compatibility. > > > > Signed-off-by: Alban Bedel > > --- > > Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 > > --- > > drivers/nvmem/core.c | 10 + > > 2 files changed, 48 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt > > b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > index fd06c09..6b723e7 100644 > > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > @@ -11,14 +11,29 @@ these data from, and where they are stored on the > > storage device. > > This document is here to document this. > > > > = Data providers = > > -Contains bindings specific to provider drivers and data cells as children > > -of this node. > > +A data provider should have a subnode named 'nvmem-cells' that contains > > +a subnodes for each data cells. > > + > > +For backward compatibility the nvmem data cells can be direct children > > +of the data provider. This use is discouraged as it can conflict with > > +other bindings. > > I don't think we need to go this far. Whether this is necessary depends > on the provider. It depend more on the drivers implementation. Sure as long as the driver only support the nvmem API it doesn't matter, both binding are fine. But if it ever need to support another API the bindings might clash and the whole device binding will need to be updated. So all in all I see very few value in still allowing the old binding for new devices, or do you seen any problem with the new binding? However if the consensus is to keep both styles I will rewrite this paragraph as needed. Alban pgplORqZJWcEN.pgp Description: OpenPGP digital signature
Re: [PATCH] phy: Add a driver for the ATH79 USB phy
On Sat, 24 Mar 2018 23:38:40 +0100 Alban Bedel <al...@free.fr> wrote: > The ATH79 USB phy is very simple, it only have a reset. On some SoC a > second reset is used to force the phy in suspend mode regardless of the > USB controller status. > > This driver is added to the qualcom directory as atheros is now part > of qualcom and newer SoC of this familly are marketed under the > qualcom name. Does this patch need any improvement or will it be applied? Alban pgpD3XbuhHG7R.pgp Description: OpenPGP digital signature
Re: [PATCH] phy: Add a driver for the ATH79 USB phy
On Sat, 24 Mar 2018 23:38:40 +0100 Alban Bedel wrote: > The ATH79 USB phy is very simple, it only have a reset. On some SoC a > second reset is used to force the phy in suspend mode regardless of the > USB controller status. > > This driver is added to the qualcom directory as atheros is now part > of qualcom and newer SoC of this familly are marketed under the > qualcom name. Does this patch need any improvement or will it be applied? Alban pgpD3XbuhHG7R.pgp Description: OpenPGP digital signature
[PATCH] [RFC][WIP] namespace.c: Allow some unprivileged proc mounts when not fully visible
Since Linux v4.2 with commit 1b852bceb0d1 ("mnt: Refactor the logic for
mounting sysfs and proc in a user namespace"), new mounts of proc or
sysfs in non init userns are only allowed when there is at least one
fully-visible proc or sysfs mount.
This is to enforce that proc/sysfs files masked by a mount are still
masked in a new mount in a unprivileged userns. The locked mount logic
for bind mounts (has_locked_children()) was not enough in the case of
proc/sysfs new mounts because some files in proc (/proc/kcore) exist as
a singleton rather than being owned by a specific proc mount.
Unfortunately, this blocks me from using userns from within a Docker
container because Docker containers mask entries such as /proc/kcore. My
use case is to build container images with arbitrary commands (such as
using "RUN" commands in Dockerfiles) without privileges and from within
a Docker container. Those arbitrary commands could be shell scripts that
require /proc.
The following commands show my problem:
$ sudo docker run -ti --rm --cap-add=SYS_ADMIN busybox sh -c 'unshare -U -r -p
-m -f mount -t proc proc /home && echo ok'
mount: permission denied (are you root?)
$ sudo docker run -ti --rm --cap-add=SYS_ADMIN busybox sh -c 'mkdir -p
/unmasked-proc && mount -t proc proc /unmasked-proc && unshare -U -r -p -m -f
mount -t proc proc /home && echo ok'
ok
This patch is a WIP attempt to ease new proc mounts in a user namespace
even when the proc mount in the parent container has masked entries.
However, to preserve the security guarantee of mount_too_revealing(),
the same masked entries in the old proc mount must be masked in the new
proc mount.
It cannot be masked with mounts covering the entries because it's not
possible to use MS_REC for new proc mount and add covering submounts at
the same time. Instead, it introduces new options in proc to disable
some proc entries (TBD). A proc entry will be disabled when all other
proc mounts have the same entry disabled, or when all other proc mounts
have the same entry masked by a submount.
The granularity does not need to be per proc entry. It is simpler to
define categories of entries that can be hidden. In practice, only a few
entries need to support disablement and what matters is that the new
proc mount is more masked than the other proc mounts. Granularity can be
improved later if use cases exist.
The flag IOP_USERNS_HIDEABLE is added on some proc inodes that are
singletons such as /proc/kcore. This flag is used in
mnt_already_visible() to signal that, as an exception to the general
rule, the file can be masked by a mount without blocking the new proc
mount. The hideable category is computed (WIP) and returned (WIP) in
order to configure the new proc mount before attaching it to the mount
tree.
For my use case, I will need to support at least the following entries:
$ sudo docker run -ti --rm busybox sh -c 'mount|grep /proc/'
proc on /proc/asound type proc (ro,nosuid,nodev,noexec,relatime)
proc on /proc/bus type proc (ro,nosuid,nodev,noexec,relatime)
proc on /proc/fs type proc (ro,nosuid,nodev,noexec,relatime)
proc on /proc/irq type proc (ro,nosuid,nodev,noexec,relatime)
proc on /proc/sys type proc (ro,nosuid,nodev,noexec,relatime)
proc on /proc/sysrq-trigger type proc (ro,nosuid,nodev,noexec,relatime)
tmpfs on /proc/kcore type tmpfs (rw,context="...",nosuid,mode=755)
tmpfs on /proc/latency_stats type tmpfs (rw,context="...",nosuid,mode=755)
tmpfs on /proc/timer_list type tmpfs (rw,context="...",nosuid,mode=755)
tmpfs on /proc/sched_debug type tmpfs (rw,context="...",nosuid,mode=755)
tmpfs on /proc/scsi type tmpfs (ro,seclabel,relatime)
This patch can be tested in the following way:
$ sudo unshare -p -f -m sh -c "mount --bind /dev/null /proc/cmdline && unshare
-U -r -p -m -f mount -t proc proc /proc && echo ok"
mount: /proc: permission denied.
(this patch does not support /proc/cmdline as hideable)
$ sudo unshare -p -f -m sh -c "mount --bind /dev/null /proc/kcore && unshare -U
-r -p -m -f mount -t proc proc /proc && echo ok"
ok
(this patch marks /proc/kcore as hideable: the new mounts works fine,
whereas it didn't work on vanilla kernels)
Signed-off-by: Alban Crequy <al...@kinvolk.io>
---
fs/namespace.c | 26 +-
fs/proc/generic.c | 5 +
fs/proc/inode.c| 2 ++
fs/proc/internal.h | 1 +
include/linux/fs.h | 1 +
5 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 9d1374ab6e06..0d466885c181 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2489,7 +2489,7 @@ static int do_add_mount(struct mount *newmnt, struct path
*path, int mnt_flags)
return err;
}
-static bool mount_too_revealing(struct vfsmount *mnt, int *new_mnt_flags);
+static bool mount_too_revealing(struct vfsmount *mnt, int *new_mnt_flags, int
*hideable
