linux-kernel - Re: [PATCH v4 06/23] mailbox: Allow controller specific mapping using fwnode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK9=C2XbNeZRp9h4FevVBm-NdPH3w5t=C1NhAn5WQAkNLK9d_A@mail.gmail.com>
Date: Mon, 9 Jun 2025 14:40:32 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jassi Brar <jassisinghbrar@...il.com>, Thomas Gleixner <tglx@...utronix.de>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Uwe Kleine-König <ukleinek@...nel.org>, 
	Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>, 
	Rahul Pathak <rpathak@...tanamicro.com>, Leyfoon Tan <leyfoon.tan@...rfivetech.com>, 
	Atish Patra <atish.patra@...ux.dev>, Andrew Jones <ajones@...tanamicro.com>, 
	Samuel Holland <samuel.holland@...ive.com>, Anup Patel <anup@...infault.org>, 
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 06/23] mailbox: Allow controller specific mapping using fwnode

On Tue, May 27, 2025 at 5:11 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Sun, May 25, 2025 at 02:16:53PM +0530, Anup Patel wrote:
> > Introduce optional fw_node() callback which allows a mailbox controller
> > driver to provide controller specific mapping using fwnode.
> >
> > The Linux OF framework already implements fwnode operations for the
> > Linux DD framework so the fw_xlate() callback works fine with device
> > tree as well.
>
> ...
>
> >  struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
> >  {
> > +     struct fwnode_reference_args fwspec;
>
> + property.h (if not done yet)

This needs fwnode.h

>
> > -     int ret;
> > +     int i, ret;
>
> Why is 'i' signed?

No particular reason because 'i' is not being assigned a negative
value so both signed and unsigned will work fine.

>
> > -     if (!dev || !dev->of_node) {
> > -             pr_debug("%s: No owner device node\n", __func__);
> > +     if (!dev || !dev->fwnode) {
>
> Do not dereference fwnode directly. Use dev_fwnode.
>
> > +             pr_debug("%s: No owner %s\n", __func__, !dev ? "device" : "fwnode");
>
> Use positive conditional.
>
> __func__ is redundant it debug messages. With Dynamic Debug enabled it may be
> switched at run-time.

Okay, I will drop the __func__

>
> >               return ERR_PTR(-ENODEV);
> >       }
> >
> > -     ret = of_parse_phandle_with_args(dev->of_node, "mboxes", "#mbox-cells",
> > -                                      index, &spec);
> > +     ret = fwnode_property_get_reference_args(dev->fwnode, "mboxes",
>
>         struct fwnode_handle *fwnode = dev_fwnode(dev);
>
> > +                                              "#mbox-cells", 0, index, &fwspec);
> >       if (ret) {
> >               dev_dbg(dev, "%s: can't parse \"mboxes\" property\n", __func__);
> >               return ERR_PTR(ret);
> >       }
>
> > +     memset(&spec, 0, sizeof(spec));
> > +     if (dev->of_node) {
>
> What is this check for?

It's a paranoid check on my part. I will drop the check
in the next revision.

>
> > +             spec.np = to_of_node(fwspec.fwnode);
> > +             spec.args_count = fwspec.nargs;
> > +             for (i = 0; i < spec.args_count; i++)
> > +                     spec.args[i] = fwspec.args[i];
> > +     }
> > +
> >       mutex_lock(&con_mutex);
> >
> >       chan = ERR_PTR(-EPROBE_DEFER);
> > -     list_for_each_entry(mbox, &mbox_cons, node)
> > -             if (mbox->dev->of_node == spec.np) {
> > +     list_for_each_entry(mbox, &mbox_cons, node) {
> > +             if (mbox->fw_xlate && mbox->dev->fwnode == fwspec.fwnode) {
> > +                     chan = mbox->fw_xlate(mbox, &fwspec);
> > +                     if (!IS_ERR(chan))
> > +                             break;
> > +             } else if (mbox->of_xlate && mbox->dev->of_node == spec.np) {
> >                       chan = mbox->of_xlate(mbox, &spec);
> >                       if (!IS_ERR(chan))
> >                               break;
> >               }
>
>
>                 if (!IS_ERR(...))
>                         break;
>
> is common.

Latest mailbox.c has changed so I will check and update accordingly.

>
>
> > +     }
>
> ...
>
> > +fw_mbox_index_xlate(struct mbox_controller *mbox,
> > +                 const struct fwnode_reference_args *sp)
>
> One line?

Okay, I will update.

Regards,
Anup