| Message ID | 1427667288-23903-1-git-send-email-zajec5@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 30, 2015 at 12:14:48AM +0200, Rafał Miłecki wrote: > Device vendors often assign IDs to their devices to allow comparing > firmware image with device model. This is required to prevent users > from flashing incompatible image and soft-bricking device. > Add device_id property to DTs to allow user space (and optionally > bootloader) verifying firmware images. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > Hi guys, > > I think my commit message explains pretty well what I'm trying to do, > however I'm not sure if I'm using a right place for that. > I also didn't document this news property, as I can't find a place > where root-properties are currenty described. Is there any such place? > I couldn't find a file describing e.g. "model" property. What does this do which the top-level "compatible" doesn't do? > diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts > index 8b62836..9d2fac4 100644 > --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts > +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts > @@ -14,6 +14,7 @@ > / { > compatible = "asus,rt-ac68u", "brcm,bcm4708"; > model = "Asus RT-AC68U (BCM4708)"; > + device_id = "RT-AC68U"; "asus,rt-ac68u" is more specific than your device ID - it's entirely possible that two manufacturers could choose the same model number for their device - matching on the model number alone is opening this up to exactly the issue you're trying to prevent. If you have your validator check for "asus,rt-ac68u" on such a device rather than the magic string "RT-AC68U", it should be much better. > diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts > index 2ed9e57..98c8d63 100644 > --- a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts > +++ b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts > @@ -14,6 +14,7 @@ > / { > compatible = "netgear,r6250v1", "brcm,bcm4708"; > model = "Netgear R6250 V1 (BCM4708)"; > + device_id = "U12H245T00_NETGEAR"; Here you decide to include the manufacturer at the end of the name. If the model number is "U12H245T00" why doesn't the compatible string reflect this? That sounds like a mistake to me. I quote the definition of the "compatible" property: Property: compatible Value type: <stringlist> Description: The compatible property value consists of one or more strings that define the specific programming model for the device. This list of strings should be used by a client program for device driver selection. The property value consists of a concatenated list of null terminated strings, from most specific to most general. They allow a device to express its compatibility with a family of similar devices, potentially allowing a single device driver to match against several devices. The recommended format is ■manufacturer,model■, where manufacturer is a string describing the name of the manufacturer (such as a stock ticker symbol), and model specifies the model number. So, checking the top-level compatible _should_ do everything you want, provided it follows the requirements of ePAPR.
On 30 March 2015 at 00:36, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Mar 30, 2015 at 12:14:48AM +0200, Rafał Miłecki wrote: >> Device vendors often assign IDs to their devices to allow comparing >> firmware image with device model. This is required to prevent users >> from flashing incompatible image and soft-bricking device. >> Add device_id property to DTs to allow user space (and optionally >> bootloader) verifying firmware images. >> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >> --- >> Hi guys, >> >> I think my commit message explains pretty well what I'm trying to do, >> however I'm not sure if I'm using a right place for that. >> I also didn't document this news property, as I can't find a place >> where root-properties are currenty described. Is there any such place? >> I couldn't find a file describing e.g. "model" property. > > What does this do which the top-level "compatible" doesn't do? Let me just answer to this one, it should make rest answers redundant. I obviously forgot to explain what was clear to me. What we use in "compatible" is some sane-looking string describing machine in a friendly way. Why my patch adds is some vendor-picked ID which is more like a MAGIC specific for a device. If you download firmware image from vendor's site (Netgear, Asus, others too), it includes some extra header/tail with a vendor picked ID (or better: MAGIC). Vendor's bootloader and default-firmware-web-UI requires that MAGIC or will reject the firmware. It also means that: 1) Generating device-compatible (and so flashable) firmware (like OpenWrt or similar) requires using the same vendor-picked MAGIC. 2) Flashing vendor-provided firmware image from custom firmware (e.g. OpenWrt) requires comparing provided file with a device-specific MAGIC. Since vendors (Netgear, Asus, others) provide firmwares with their own picked MAGICs, we can't compare them against "asus,rt-ac68u", "netgear,r6250v1", or whatever we use. That also means comparing it against some more-or-less silly strings like "U12H245T00_NETGEAR". Let me know if that explains my purpose of device_id a bit.
On Mon, Mar 30, 2015 at 12:54:27AM +0200, Rafał Miłecki wrote: > On 30 March 2015 at 00:36, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, Mar 30, 2015 at 12:14:48AM +0200, Rafał Miłecki wrote: > >> Device vendors often assign IDs to their devices to allow comparing > >> firmware image with device model. This is required to prevent users > >> from flashing incompatible image and soft-bricking device. > >> Add device_id property to DTs to allow user space (and optionally > >> bootloader) verifying firmware images. > >> > >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > >> --- > >> Hi guys, > >> > >> I think my commit message explains pretty well what I'm trying to do, > >> however I'm not sure if I'm using a right place for that. > >> I also didn't document this news property, as I can't find a place > >> where root-properties are currenty described. Is there any such place? > >> I couldn't find a file describing e.g. "model" property. > > > > What does this do which the top-level "compatible" doesn't do? > > Let me just answer to this one, it should make rest answers redundant. > I obviously forgot to explain what was clear to me. > > What we use in "compatible" is some sane-looking string describing > machine in a friendly way. Why my patch adds is some vendor-picked ID > which is more like a MAGIC specific for a device. > > If you download firmware image from vendor's site (Netgear, Asus, > others too), it includes some extra header/tail with a vendor picked > ID (or better: MAGIC). Vendor's bootloader and default-firmware-web-UI > requires that MAGIC or will reject the firmware. It also means that: > > 1) Generating device-compatible (and so flashable) firmware (like > OpenWrt or similar) requires using the same vendor-picked MAGIC. > > 2) Flashing vendor-provided firmware image from custom firmware (e.g. > OpenWrt) requires comparing provided file with a device-specific > MAGIC. Since vendors (Netgear, Asus, others) provide firmwares with > their own picked MAGICs, we can't compare them against > "asus,rt-ac68u", "netgear,r6250v1", or whatever we use. That also > means comparing it against some more-or-less silly strings like > "U12H245T00_NETGEAR". > > Let me know if that explains my purpose of device_id a bit. Yes. Now you need to add a description of the new property to the appropriate place in Documentation/devicetree/bindings first before you can add it to the device tree descriptions themselves, and have the DT maintainers review your new description. You'll also want to read Documentation/devicetree/bindings/submitting-patches.txt before making your submission. I'd suggest explaining it fully - maybe this would be better: Device vendors often assign their own IDs to their devices to allow comparing the firmware image with device model. This is required to prevent users from flashing an incompatible image and soft-bricking device. Add a device_id property to contain this vendor assigned ID, which is intended to inform userspace of the appropriate ID to verify in user supplied firmware images prior to programming. This makes it clear that the purpose here is to supply an identifier to userspace in order to verify user supplied firmware images, rather than this property being used to identify a firmware image. I'll leave it up to DT people to decide whether this is something they want to accept; they'll probably only comment with a properly submitted binding proposal. Thanks.
Hi, For DT patches, please send to devicetree@vger.kernel.org; this affects more than just ARM. On Sun, Mar 29, 2015 at 11:14:48PM +0100, Rafał Miłecki wrote: > Device vendors often assign IDs to their devices to allow comparing > firmware image with device model. This is required to prevent users > from flashing incompatible image and soft-bricking device. > Add device_id property to DTs to allow user space (and optionally > bootloader) verifying firmware images. This sounds like exactly what the "model" property is meant to be (per ePAPR) -- a string that specifices the manufacturer's model number of the device, ideally in "manufacturer,model" format. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > Hi guys, > > I think my commit message explains pretty well what I'm trying to do, > however I'm not sure if I'm using a right place for that. > I also didn't document this news property, as I can't find a place > where root-properties are currenty described. Is there any such place? > I couldn't find a file describing e.g. "model" property. It's documented in ePAPR (embedded power achitecture platform requirements), and possibly earlier device tree and/or open firmware documents. Unfortuantely ePAPR is now behind a registration page on power.org, and I'm not sure where most other documents live (playground.sun.com and other sources are dead). Mark. > --- > arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts | 1 + > arch/arm/boot/dts/bcm4708-netgear-r6250.dts | 1 + > arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts | 1 + > arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts | 1 + > arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 1 + > 5 files changed, 5 insertions(+) > > diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts > index 8b62836..9d2fac4 100644 > --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts > +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts > @@ -14,6 +14,7 @@ > / { > compatible = "asus,rt-ac68u", "brcm,bcm4708"; > model = "Asus RT-AC68U (BCM4708)"; > + device_id = "RT-AC68U"; > > chosen { > bootargs = "console=ttyS0,115200"; > diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts > index 2ed9e57..98c8d63 100644 > --- a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts > +++ b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts > @@ -14,6 +14,7 @@ > / { > compatible = "netgear,r6250v1", "brcm,bcm4708"; > model = "Netgear R6250 V1 (BCM4708)"; > + device_id = "U12H245T00_NETGEAR"; > > chosen { > bootargs = "console=ttyS0,115200"; > diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts > index 3991042..c43173a 100644 > --- a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts > +++ b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts > @@ -14,6 +14,7 @@ > / { > compatible = "netgear,r6300v2", "brcm,bcm4708"; > model = "Netgear R6300 V2 (BCM4708)"; > + device_id = "U12H240T00_NETGEAR"; > > chosen { > bootargs = "console=ttyS0,115200"; > diff --git a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts > index 0ee85ea..876e2c0 100644 > --- a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts > +++ b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts > @@ -14,6 +14,7 @@ > / { > compatible = "asus,rt-n18u", "brcm,bcm47081", "brcm,bcm4708"; > model = "Asus RT-N18U (BCM47081)"; > + device_id = "RT-N18U"; > > chosen { > bootargs = "console=ttyS0,115200"; > diff --git a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts > index ea26dd3..129df9a 100644 > --- a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts > +++ b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts > @@ -14,6 +14,7 @@ > / { > compatible = "netgear,r8000", "brcm,bcm4709", "brcm,bcm4708"; > model = "Netgear R8000 (BCM4709)"; > + device_id = "U12H315T00_NETGEAR"; > > chosen { > bootargs = "console=ttyS0,115200"; > -- > 1.8.4.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 30 March 2015 at 12:37, Mark Rutland <mark.rutland@arm.com> wrote: > For DT patches, please send to devicetree@vger.kernel.org; this affects > more than just ARM. Ofc, sorry. > On Sun, Mar 29, 2015 at 11:14:48PM +0100, Rafał Miłecki wrote: >> Device vendors often assign IDs to their devices to allow comparing >> firmware image with device model. This is required to prevent users >> from flashing incompatible image and soft-bricking device. >> Add device_id property to DTs to allow user space (and optionally >> bootloader) verifying firmware images. > > This sounds like exactly what the "model" property is meant to be (per > ePAPR) -- a string that specifices the manufacturer's model number of > the device, ideally in "manufacturer,model" format. What if manufacturer decided to use some totally unfriendly ID for their low-level (firmware upgrade) model identification? I can't really see us using model = "U12H245T00_NETGEAR" which would mean nothing compared to the current friendly: model = "Netgear R6250 V1 (BCM4708)"
> > On Sun, Mar 29, 2015 at 11:14:48PM +0100, Rafał Miłecki wrote: > >> Device vendors often assign IDs to their devices to allow comparing > >> firmware image with device model. This is required to prevent users > >> from flashing incompatible image and soft-bricking device. > >> Add device_id property to DTs to allow user space (and optionally > >> bootloader) verifying firmware images. > > > > This sounds like exactly what the "model" property is meant to be (per > > ePAPR) -- a string that specifices the manufacturer's model number of > > the device, ideally in "manufacturer,model" format. > > What if manufacturer decided to use some totally unfriendly ID for > their low-level (firmware upgrade) model identification? I can't > really see us using > model = "U12H245T00_NETGEAR" > which would mean nothing compared to the current friendly: > model = "Netgear R6250 V1 (BCM4708)" If it uniquely identifies the model, it's fit to be a model string. If being "friendly" means that we lose that, then the property is useless anyway. Note that both examples above deviate from the recommended format, and something like: "netgear,U12H245T00" would better align with the recommendation. Mark.
On Monday 30 March 2015 11:58:29 Mark Rutland wrote: > > > On Sun, Mar 29, 2015 at 11:14:48PM +0100, Rafał Miłecki wrote: > > >> Device vendors often assign IDs to their devices to allow comparing > > >> firmware image with device model. This is required to prevent users > > >> from flashing incompatible image and soft-bricking device. > > >> Add device_id property to DTs to allow user space (and optionally > > >> bootloader) verifying firmware images. > > > > > > This sounds like exactly what the "model" property is meant to be (per > > > ePAPR) -- a string that specifices the manufacturer's model number of > > > the device, ideally in "manufacturer,model" format. > > > > What if manufacturer decided to use some totally unfriendly ID for > > their low-level (firmware upgrade) model identification? I can't > > really see us using > > model "U12H245T00_NETGEAR" > > which would mean nothing compared to the current friendly: > > model "Netgear R6250 V1 (BCM4708)" > > If it uniquely identifies the model, it's fit to be a model string. If > being "friendly" means that we lose that, then the property is useless > anyway. > > Note that both examples above deviate from the recommended format, and > something like: "netgear,U12H245T00" would better align with the > recommendation. Sorry for jumping in late here, but this still sounds like more fitting for the "compatible" property than "model". For compatible, you can already have multiple strings, and they come with a vendor prefix. Any of these would seem reasonable to me: a) compatible = "asus,rt-ac68u", "openwrt,RT-AC68U", "brcm,bcm4708"; model = "Asus RT-AC68U (BCM4708)"; b) compatible = "asus,rt-ac68u", "brcm,RT-AC68U", "brcm,bcm4708"; model = "Asus RT-AC68U (BCM4708)"; c) compatible = "asus,rt-ac68u", "asus,RT-AC68U", "brcm,bcm4708"; model = "Asus RT-AC68U (BCM4708)"; depending on which party maintains the list of these identifiers. Arnd
On 10 April 2015 at 12:50, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 30 March 2015 11:58:29 Mark Rutland wrote: >> > > On Sun, Mar 29, 2015 at 11:14:48PM +0100, Rafał Miłecki wrote: >> > >> Device vendors often assign IDs to their devices to allow comparing >> > >> firmware image with device model. This is required to prevent users >> > >> from flashing incompatible image and soft-bricking device. >> > >> Add device_id property to DTs to allow user space (and optionally >> > >> bootloader) verifying firmware images. >> > > >> > > This sounds like exactly what the "model" property is meant to be (per >> > > ePAPR) -- a string that specifices the manufacturer's model number of >> > > the device, ideally in "manufacturer,model" format. >> > >> > What if manufacturer decided to use some totally unfriendly ID for >> > their low-level (firmware upgrade) model identification? I can't >> > really see us using >> > model "U12H245T00_NETGEAR" >> > which would mean nothing compared to the current friendly: >> > model "Netgear R6250 V1 (BCM4708)" >> >> If it uniquely identifies the model, it's fit to be a model string. If >> being "friendly" means that we lose that, then the property is useless >> anyway. >> >> Note that both examples above deviate from the recommended format, and >> something like: "netgear,U12H245T00" would better align with the >> recommendation. > > Sorry for jumping in late here, but this still sounds like more fitting > for the "compatible" property than "model". For compatible, you can > already have multiple strings, and they come with a vendor prefix. > > Any of these would seem reasonable to me: > > a) > compatible = "asus,rt-ac68u", "openwrt,RT-AC68U", "brcm,bcm4708"; > model = "Asus RT-AC68U (BCM4708)"; > > b) > compatible = "asus,rt-ac68u", "brcm,RT-AC68U", "brcm,bcm4708"; > model = "Asus RT-AC68U (BCM4708)"; > > c) > compatible = "asus,rt-ac68u", "asus,RT-AC68U", "brcm,bcm4708"; > model = "Asus RT-AC68U (BCM4708)"; > > depending on which party maintains the list of these identifiers. Thanks, I think it looks like the best alternative to my initial idea.
diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts index 8b62836..9d2fac4 100644 --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts @@ -14,6 +14,7 @@ / { compatible = "asus,rt-ac68u", "brcm,bcm4708"; model = "Asus RT-AC68U (BCM4708)"; + device_id = "RT-AC68U"; chosen { bootargs = "console=ttyS0,115200"; diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts index 2ed9e57..98c8d63 100644 --- a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts +++ b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts @@ -14,6 +14,7 @@ / { compatible = "netgear,r6250v1", "brcm,bcm4708"; model = "Netgear R6250 V1 (BCM4708)"; + device_id = "U12H245T00_NETGEAR"; chosen { bootargs = "console=ttyS0,115200"; diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts index 3991042..c43173a 100644 --- a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts +++ b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts @@ -14,6 +14,7 @@ / { compatible = "netgear,r6300v2", "brcm,bcm4708"; model = "Netgear R6300 V2 (BCM4708)"; + device_id = "U12H240T00_NETGEAR"; chosen { bootargs = "console=ttyS0,115200"; diff --git a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts index 0ee85ea..876e2c0 100644 --- a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts +++ b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts @@ -14,6 +14,7 @@ / { compatible = "asus,rt-n18u", "brcm,bcm47081", "brcm,bcm4708"; model = "Asus RT-N18U (BCM47081)"; + device_id = "RT-N18U"; chosen { bootargs = "console=ttyS0,115200"; diff --git a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts index ea26dd3..129df9a 100644 --- a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts +++ b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts @@ -14,6 +14,7 @@ / { compatible = "netgear,r8000", "brcm,bcm4709", "brcm,bcm4708"; model = "Netgear R8000 (BCM4709)"; + device_id = "U12H315T00_NETGEAR"; chosen { bootargs = "console=ttyS0,115200";
Device vendors often assign IDs to their devices to allow comparing firmware image with device model. This is required to prevent users from flashing incompatible image and soft-bricking device. Add device_id property to DTs to allow user space (and optionally bootloader) verifying firmware images. Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- Hi guys, I think my commit message explains pretty well what I'm trying to do, however I'm not sure if I'm using a right place for that. I also didn't document this news property, as I can't find a place where root-properties are currenty described. Is there any such place? I couldn't find a file describing e.g. "model" property. --- arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts | 1 + arch/arm/boot/dts/bcm4708-netgear-r6250.dts | 1 + arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts | 1 + arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts | 1 + arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 1 + 5 files changed, 5 insertions(+)