Re: [PATCH v4 02/10] mei: late_bind: add late binding component driver

[Rodrigo Vivi]

On Tue, Jul 01, 2025 at 10:11:54PM +0530, Nilawar, Badal wrote:
> 
> On 01-07-2025 18:04, Nilawar, Badal wrote:
> > 
> > On 01-07-2025 15:15, Greg KH wrote:
> > > On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
> > > > On 28-06-2025 17:48, Greg KH wrote:
> > > > > > + * @payload_size: size of the payload data in bytes
> > > > > > + * @payload: data to be sent to the firmware
> > > > > > + */
> > > > > > +struct csc_heci_late_bind_req {
> > > > > > +    struct mkhi_msg_hdr header;
> > > > > > +    u32 type;
> > > > > > +    u32 flags;
> > > > > What is the endian of these fields?  And as this crosses the
> > > > > kernel/hardware boundry, shouldn't these be __u32?
> > > > endian of these fields is little endian, all the headers are
> > > > little endian.
> > > > I will add comment at top.
> > > No, use the proper types if this is little endian.  Don't rely on a
> > > comment to catch things when it goes wrong.
> > > 
> > > > On __u32 I doubt we need to do it as csc send copy it to
> > > > internal buffer.
> > > If this crosses the kernel boundry, it needs to use the proper type.
> > 
> > Understood. I will proceed with using __le32 in this context, provided
> > that Sasha agrees.
> 
> I believe __le{32, 16} is used only when the byte order is fixed and matches
> the host system's native endianness. Since the CSC controller is
> little-endian, is it necessary to specify the endianness here?
> If it is mandatory to use the __le{32, 16} endian type, then is there need
> to convert endianness using cpu_to_le and le_to_cpu?

I honestly don't believe that specifying endianness here is **needed**.
I mean, it might be future safe to use the __le32 and
flags = cpu_to_le32(1 << 0) just in case someone decide to port all the
GPU code to run in big-endian CPU. Very unlikely I'd say, and much more cases
to resolve before we get to this gpu use case here I'm afraid.

Weel, unless this mei here can be used outside of GPU context?!

> 
> > 
> > > 
> > > > > > +{
> > > > > > +    struct mei_cl_device *cldev;
> > > > > > +    struct csc_heci_late_bind_req *req = NULL;
> > > > > > +    struct csc_heci_late_bind_rsp rsp;
> > > > > > +    size_t req_size;
> > > > > > +    ssize_t ret;
> > > > > > +
> > > > > > +    if (!dev || !payload || !payload_size)
> > > > > > +        return -EINVAL;
> > > > > How can any of these ever happen as you control the callers of this
> > > > > function?
> > > > I will add WARN here.
> > > So you will end up crashing the machine and getting a CVE assigned for
> > > it?
> > > 
> > > Please no.  If it can't happen, then don't check for it.  If it can
> > > happen, great, handle it properly.  Don't just give up and cause a
> > > system to reboot, that's a horrible way to write kernel code.

I agree here that the WARN is not a good way to handle that.
We either don't check (remove it) or handle properly (keep as is).

With the context of where this driver is used I'd say it can't happen.
Since xe is properly setting it right now and I don't believe we have
other usages of this mei driver here.

But if there's a chance of this getting used outside of xe, then
we need to keep the check...

But if you keep the check, then also use __lb32() because we need
some consistency in the reasoning, one way or the other.

> > 
> > Fine, will drop the idea of WARN here.
> > 
> > Thanks,
> > Badal
> > 
> > > 
> > > thanks,
> > > 
> > > greg k-h