Re: [PATCH] of: give priority to the compatible match in __of_match_node()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, 19 Feb 2014 16:23:02 -0500, Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx> wrote:
> On 14-02-19 03:41 PM, Grant Likely wrote:
> > On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx> wrote:
> >> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@xxxxxxxxx> wrote:
> >>> On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@xxxxxxxxx> wrote:
> >>>> When the device node do have a compatible property, we definitely
> >>>> prefer the compatible match besides the type and name. Only if
> >>>> there is no such a match, we then consider the candidate which
> >>>> doesn't have compatible entry but do match the type or name with
> >>>> the device node.
> >>>>
> >>>> This is based on a patch from Sebastian Hesselbarth.
> >>>>   http://patchwork.ozlabs.org/patch/319434/
> >>>>
> >>>> I did some code refactoring and also fixed a bug in the original patch.
> >>>
> >>> I'm inclined to just revert this once again and avoid possibly
> >>> breaking yet another platform.
> >>
> >> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot
> >> on my sbc8548.   It fails with:
> > 
> > I think I've got it fixed now with the latest series. Please try the
> > devicetree/merge branch on git://git.secretlab.ca/git/linux
> 
> That seems to work.

Great, thanks for the testing. Can I add a Tested-by line for you?

g.

> 
> libphy: Freescale PowerQUICC MII Bus: probed
> libphy: Freescale PowerQUICC MII Bus: probed
> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
> TCP: cubic registered
> Initializing XFRM netlink socket
> NET: Registered protocol family 17
> 
> ...
> 
> root@sbc8548:~# cat /proc/version 
> Linux version 3.14.0-rc3-00024-g7119f42057c6 (paul@yow-lpgnfs-02) (gcc version 4.5.2 (GCC) ) #1 Wed Feb 19 16:08:17 EST 2014
> root@sbc8548:~#
> 
> Paul.
> --
> 
> > 
> > g.
> > 
> >> -----------------------------------------------
> >> libphy: Freescale PowerQUICC MII Bus: probed
> >> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520
> >> PHY address 1312 is too large
> >> libphy: Freescale PowerQUICC MII Bus: probed
> >> libphy: Freescale PowerQUICC MII Bus: probed
> >> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520
> >> PHY address 1312 is too large
> >> libphy: Freescale PowerQUICC MII Bus: probed
> >> TCP: cubic registered
> >> Initializing XFRM netlink socket
> >> NET: Registered protocol family 17
> >> <fail nfs mount>
> >> -----------------------------------------------
> >>
> >> On a normal boot, we should see this:
> >> -----------------------------------------------
> >> libphy: Freescale PowerQUICC MII Bus: probed
> >> libphy: Freescale PowerQUICC MII Bus: probed
> >> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
> >> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
> >> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
> >> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
> >> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
> >> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
> >> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
> >> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
> >> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
> >> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
> >> TCP: cubic registered
> >> Initializing XFRM netlink socket
> >> NET: Registered protocol family 17
> >> -----------------------------------------------
> >>
> >>
> >> Git bisect says:
> >>
> >> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit
> >> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4
> >> Author: Kevin Hao <haokexin@xxxxxxxxx>
> >> Date:   Tue Feb 18 15:57:30 2014 +0800
> >>
> >>     of: reimplement the matching method for __of_match_node()
> >>
> >>     In the current implementation of __of_match_node(), it will compare
> >>     each given match entry against all the node's compatible strings
> >>     with of_device_is_compatible().
> >>
> >>     To achieve multiple compatible strings per node with ordering from
> >>     specific to generic, this requires given matches to be ordered from
> >>     specific to generic. For most of the drivers this is not true and
> >>     also an alphabetical ordering is more sane there.
> >>
> >>     Therefore, we define a following priority order for the match, and
> >>     then scan all the entries to find the best match.
> >>       1. specific compatible && type && name
> >>       2. specific compatible && type
> >>       3. specific compatible && name
> >>       4. specific compatible
> >>       5. general compatible && type && name
> >>       6. general compatible && type
> >>       7. general compatible && name
> >>       8. general compatible
> >>       9. type && name
> >>       10. type
> >>       11. name
> >>
> >>     This is based on some pseudo-codes provided by Grant Likely.
> >>
> >>     Signed-off-by: Kevin Hao <haokexin@xxxxxxxxx>
> >>     [grant.likely: Changed multiplier to 4 which makes more sense]
> >>     Signed-off-by: Grant Likely <grant.likely@xxxxxxxxxx>
> >>
> >> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0
> >> 8401b0e3903e23e973845ee75b26b04345d803d2 M      drivers
> >>
> >> As a double check, I checked out the head of linux-next, and did a
> >> revert of the above commit, and my sbc8548 can then boot properly.
> >>
> >> Not doing anything fancy ; using the defconfig exactly as-is, and
> >> ensuring the dtb is fresh from linux-next HEAD of today.
> >>
> >> Thanks,
> >> Paul.
> >> --
> >>
> >>>
> >>> However, I think I would like to see this structured differently. We
> >>> basically have 2 ways of matching: the existing pre-3.14 way and the
> >>> desired match on best compatible only. All new bindings should match
> >>> with the new way and the old way needs to be kept for compatibility.
> >>> So lets structure the code that way. Search the match table first for
> >>> best compatible with name and type NULL, then search the table the old
> >>> way. I realize it appears you are doing this, but it is not clear this
> >>> is the intent of the code. I would like to see this written as a patch
> >>> with commit 105353145eafb3ea919 reverted first and you add a new match
> >>> function to call first and then fallback to the existing function.
> >>>
> >>> Rob
> >>>
> >>>>
> >>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx>
> >>>> Signed-off-by: Kevin Hao <haokexin@xxxxxxxxx>
> >>>> ---
> >>>>  drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------
> >>>>  1 file changed, 37 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >>>> index ff85450d5683..9d655df458bd 100644
> >>>> --- a/drivers/of/base.c
> >>>> +++ b/drivers/of/base.c
> >>>> @@ -730,32 +730,45 @@ out:
> >>>>  }
> >>>>  EXPORT_SYMBOL(of_find_node_with_property);
> >>>>
> >>>> +static int of_match_type_or_name(const struct device_node *node,
> >>>> +                               const struct of_device_id *m)
> >>>> +{
> >>>> +       int match = 1;
> >>>> +
> >>>> +       if (m->name[0])
> >>>> +               match &= node->name && !strcmp(m->name, node->name);
> >>>> +
> >>>> +       if (m->type[0])
> >>>> +               match &= node->type && !strcmp(m->type, node->type);
> >>>> +
> >>>> +       return match;
> >>>> +}
> >>>> +
> >>>>  static
> >>>>  const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> >>>>                                            const struct device_node *node)
> >>>>  {
> >>>>         const char *cp;
> >>>>         int cplen, l;
> >>>> +       const struct of_device_id *m;
> >>>> +       int match;
> >>>>
> >>>>         if (!matches)
> >>>>                 return NULL;
> >>>>
> >>>>         cp = __of_get_property(node, "compatible", &cplen);
> >>>> -       do {
> >>>> -               const struct of_device_id *m = matches;
> >>>> +       while (cp && (cplen > 0)) {
> >>>> +               m = matches;
> >>>>
> >>>>                 /* Check against matches with current compatible string */
> >>>>                 while (m->name[0] || m->type[0] || m->compatible[0]) {
> >>>> -                       int match = 1;
> >>>> -                       if (m->name[0])
> >>>> -                               match &= node->name
> >>>> -                                       && !strcmp(m->name, node->name);
> >>>> -                       if (m->type[0])
> >>>> -                               match &= node->type
> >>>> -                                       && !strcmp(m->type, node->type);
> >>>> -                       if (m->compatible[0])
> >>>> -                               match &= cp
> >>>> -                                       && !of_compat_cmp(m->compatible, cp,
> >>>> +                       if (!m->compatible[0]) {
> >>>> +                               m++;
> >>>> +                               continue;
> >>>> +                       }
> >>>> +
> >>>> +                       match = of_match_type_or_name(node, m);
> >>>> +                       match &= cp && !of_compat_cmp(m->compatible, cp,
> >>>>                                                         strlen(m->compatible));
> >>>>                         if (match)
> >>>>                                 return m;
> >>>> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> >>>>                 }
> >>>>
> >>>>                 /* Get node's next compatible string */
> >>>> -               if (cp) {
> >>>> -                       l = strlen(cp) + 1;
> >>>> -                       cp += l;
> >>>> -                       cplen -= l;
> >>>> -               }
> >>>> -       } while (cp && (cplen > 0));
> >>>> +               l = strlen(cp) + 1;
> >>>> +               cp += l;
> >>>> +               cplen -= l;
> >>>> +       }
> >>>> +
> >>>> +       m = matches;
> >>>> +       /* Check against matches without compatible string */
> >>>> +       while (m->name[0] || m->type[0] || m->compatible[0]) {
> >>>> +               if (!m->compatible[0] && of_match_type_or_name(node, m))
> >>>> +                       return m;
> >>>> +               m++;
> >>>> +       }
> >>>>
> >>>>         return NULL;
> >>>>  }
> >>>> --
> >>>> 1.8.5.3
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>> _______________________________________________
> >>> Linuxppc-dev mailing list
> >>> Linuxppc-dev@xxxxxxxxxxxxxxxx
> >>> https://lists.ozlabs.org/listinfo/linuxppc-dev
> > 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux