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

[Du, Bin]

Thanks Sultan.

On 9/25/2025 3:20 PM, Sultan Alsawaf wrote:
> 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

Thank you for the detailed explanation. Your reasoning is both 
professional and persuasive. Will switch to using memset instead of { } 
initialization for structures that are shared with firmware. 
Additionally, will include comments before the memset call to clarify 
that it is used to ensure all padding bits are properly zeroed.

> Sultan

--
Regards,
Bin