Re: [PATCH v4 07/23] mailbox: Add RISC-V SBI message proxy (MPXY) based mailbox driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Jun 09, 2025 at 05:59:40PM +0530, Anup Patel wrote:
> On Wed, May 28, 2025 at 4:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, May 25, 2025 at 02:16:54PM +0530, Anup Patel wrote:

...

> > > +#include <asm/sbi.h>
> >
> > asm/* usually goes after generic linux/* ones. Why here?
> 
> I am not aware of any such convention but I will update anyway.

It's just a common sense. We include most generic first and most custom at
last.

...

> > > +static int mpxy_write_attrs(u32 channel_id, u32 base_attrid, u32 attr_count,
> > > +                         u32 *attrs_buf)
> > > +{
> > > +     struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
> > > +     struct sbiret sret;
> > > +     u32 i;
> > > +
> > > +     if (!mpxy->shmem_active)
> > > +             return -ENODEV;
> > > +     if (!attr_count || !attrs_buf)
> > > +             return -EINVAL;
> > > +
> > > +     get_cpu();
> > > +
> > > +     for (i = 0; i < attr_count; i++)
> > > +             ((__le32 *)mpxy->shmem)[i] = cpu_to_le32(attrs_buf[i]);
> >
> > Don't we have helpers for this? They are suffixed with _array.
> > https://elixir.bootlin.com/linux/v6.15-rc6/source/include/linux/byteorder/generic.h#L168
> > Don't forget to have asm/byteorder.h being included.
> >
> > Ditto for the similar case(s).
> 
> The cpu_to_le32_array() and le32_to_cpu_array() helpers update data
> in-place but over here we have separate source and destination.

Fair enough. Perhaps add something like memcpy_to_le32() / memcpy_from_le32()
or alike for your case?

> > > +     sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_WRITE_ATTRS,
> > > +                      channel_id, base_attrid, attr_count, 0, 0, 0);
> > > +
> > > +     put_cpu();
> > > +     return sbi_err_map_linux_errno(sret.error);
> > > +}

...

> > > +                            sizeof(mchan->rpmi_attrs) / sizeof(u32),
> > > +                            (u32 *)&mchan->rpmi_attrs);
> >
> > Why casting? What about alignment?
> 
> The RPMI attributes (aka struct sbi_mpxy_rpmi_channel_attrs) are
> a collection of u32 attributes hence we can also treat rpmi_attrs
> as a u32 array. Further, the rpmi_attrs is XLEN aligned within the
> struct mpxy_mbox_channel so no alignment issue with the casting
> on both RV32 and RV64.
> 
> If we want to avoid the casting then we will have to use a temporary
> u32 array plus additional memcpy().

OK.

...

> > > +     if (mbox->msi_count)
> >
> > Is this check really needed?
> 
> MSIs are optional for the SBI MPXY mailbox so we should only use
> platform_device_msi_xyz() APIs only when MSIs are available.

> > > +             platform_device_msi_free_irqs_all(mbox->dev);

Hmm... I am not sure why. Do you have any Oops or warnings if the check
is not there and no MSI provided?

> > > +}

-- 
With Best Regards,
Andy Shevchenko