Re: [PATCH v4 3/7] media: platform: amd: Add isp4 fw and hw interface

[Sultan Alsawaf <sultan@kerneltoast.com>]

On Thu, Sep 25, 2025 at 11:56:13AM +0800, Du, Bin wrote:
> On 9/24/2025 3:09 PM, Sultan Alsawaf wrote:
> > On Tue, Sep 23, 2025 at 05:24:11PM +0800, Du, Bin wrote:
> > > On 9/22/2025 5:55 AM, Sultan Alsawaf wrote:
> > > > On Thu, Sep 11, 2025 at 06:08:43PM +0800, Bin Du wrote:
> > > > > +	struct isp4if_cmd_element *cmd_ele = NULL;
> > > > > +	struct isp4if_rb_config *rb_config;
> > > > > +	struct device *dev = ispif->dev;
> > > > > +	struct isp4fw_cmd cmd = {};
> > > > 
> > > > Use memset() to guarantee padding bits of cmd are zeroed, since this may not
> > > > guarantee it on all compilers.
> > > > 
> > > 
> > > Sure, will do it in the next version. Just a question, padding bits seem
> > > never to be used, will it cause any problem if they are not zeroed?
> > 
> > Padding bits, if there are any, are used by isp4if_compute_check_sum() and are
> > also sent to the firmware.
> > 
> 
> Yes, this will impact the checksum value. However, based on my
> understanding, it will not affect the error detection outcome, since the
> firmware uses the same padding bits during both checksum calculation and
> comparison. I apologize for the minor disagreement—I just want to avoid
> introducing redundant code, especially given that similar scenarios appear a
> lot. Originally, we used memset in the initial version, but switched to { }
> initialization in subsequent versions based on review feedback. Please feel
> free to share your ideas, if you believe it is still necessary, we will add
> them.

Ah, I see Sakari suggested that during a prior review [1].

Whenever a struct is sent outside of the kernel, padding bits should be zeroed
for a few reasons:

1. Uninitialized padding bits can expose sensitive information from kernel
   memory, which can be a security concern.

2. There is no guarantee that the recipient will always behave the same way with
   different values for the padding bits. In this case for example, I cannot
   look at the ISP source code and say for sure that the padding bits don't
   affect its operation. And even if I could, that may always change with a new
   firmware version.

3. You can ensure more reliable testing results by guaranteeing that the padding
   bits are the same value (zero) for everyone. For example, if the padding bits
   accidentally affected the firmware, some users with different padding bits
   values could experience bugs that you cannot reproduce in your lab or dev
   environment.

The only way to ensure padding bits are zeroed on all compilers is to use
memset; using { } won't do this on every compiler or every compiler version or
even every compiler optimization level [2].

So I still believe it is necessary to use memset for those structs which are
sent outside of the kernel, in this case for the structs sent to firmware. For
structs which are used _only inside_ the kernel, it is preferred to use { }.

[1] https://lore.kernel.org/all/aIclcwRep3F_z7PF@kekkonen.localdomain/
[2] https://interrupt.memfault.com/blog/c-struct-padding-initialization#strategy-4---gcc-extension

Sultan