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

["Du, Bin" <bin.du@amd.com>]

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