| Message ID | 7ec94789-305c-4de4-b477-c0eb839170e5@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [next] media: synopsys: hdmirx: Fix signedness bug in hdmirx_parse_dt() | expand |
On 3/7/25 12:30, Dan Carpenter wrote: > The num_clks is set this way: > > hdmirx_dev->num_clks = devm_clk_bulk_get_all(dev, &hdmirx_dev->clks); > if (hdmirx_dev->num_clks < 1) > return -ENODEV; > > The devm_clk_bulk_get_all() function returns negative error codes so the > hdmirx_dev->num_cks variable needs to be signed for the error handling to > work. > > Fixes: 7b59b132ad43 ("media: platform: synopsys: Add support for HDMI input driver") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > index 4ffc86ad6c35..e0d3fed87a92 100644 > --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > @@ -154,7 +154,7 @@ struct snps_hdmirx_dev { > bool hpd_trigger_level_high; > bool tmds_clk_ratio; > bool plugged; > - u32 num_clks; > + int num_clks; > u32 edid_blocks_written; > u32 cur_fmt_fourcc; > u32 color_depth; Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Would be also good to return the original error code. There is no need to check for the < 1 clock, it should be the < 0 check. Can be done in a separate patch later. Thanks for the fix!
On Fri, Mar 07, 2025 at 12:36:47PM +0300, Dmitry Osipenko wrote: > On 3/7/25 12:30, Dan Carpenter wrote: > > diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > > index 4ffc86ad6c35..e0d3fed87a92 100644 > > --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > > +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > > @@ -154,7 +154,7 @@ struct snps_hdmirx_dev { > > bool hpd_trigger_level_high; > > bool tmds_clk_ratio; > > bool plugged; > > - u32 num_clks; > > + int num_clks; > > u32 edid_blocks_written; > > u32 cur_fmt_fourcc; > > u32 color_depth; > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Would be also good to return the original error code. There is no need > to check for the < 1 clock, it should be the < 0 check. Can be done in a > separate patch later. Thanks for the fix! I'm not very familiar with th edevm_clk_bulk_get_all() function and it's not documented. But clk_bulk_get_all() does return zero, so I can see why people would be confused. regards, dan carpenter
On 3/7/25 12:45, Dan Carpenter wrote: > On Fri, Mar 07, 2025 at 12:36:47PM +0300, Dmitry Osipenko wrote: >> On 3/7/25 12:30, Dan Carpenter wrote: >>> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>> index 4ffc86ad6c35..e0d3fed87a92 100644 >>> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>> @@ -154,7 +154,7 @@ struct snps_hdmirx_dev { >>> bool hpd_trigger_level_high; >>> bool tmds_clk_ratio; >>> bool plugged; >>> - u32 num_clks; >>> + int num_clks; >>> u32 edid_blocks_written; >>> u32 cur_fmt_fourcc; >>> u32 color_depth; >> >> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> >> Would be also good to return the original error code. There is no need >> to check for the < 1 clock, it should be the < 0 check. Can be done in a >> separate patch later. Thanks for the fix! > > I'm not very familiar with th edevm_clk_bulk_get_all() function and it's > not documented. But clk_bulk_get_all() does return zero, so I can see why > people would be confused. We will take care of it, thanks again.
On 07/03/2025 10:47, Dmitry Osipenko wrote: > On 3/7/25 12:45, Dan Carpenter wrote: >> On Fri, Mar 07, 2025 at 12:36:47PM +0300, Dmitry Osipenko wrote: >>> On 3/7/25 12:30, Dan Carpenter wrote: >>>> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>>> index 4ffc86ad6c35..e0d3fed87a92 100644 >>>> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>>> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>>> @@ -154,7 +154,7 @@ struct snps_hdmirx_dev { >>>> bool hpd_trigger_level_high; >>>> bool tmds_clk_ratio; >>>> bool plugged; >>>> - u32 num_clks; >>>> + int num_clks; >>>> u32 edid_blocks_written; >>>> u32 cur_fmt_fourcc; >>>> u32 color_depth; >>> >>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> >>> Would be also good to return the original error code. There is no need >>> to check for the < 1 clock, it should be the < 0 check. Can be done in a >>> separate patch later. Thanks for the fix! >> >> I'm not very familiar with th edevm_clk_bulk_get_all() function and it's >> not documented. But clk_bulk_get_all() does return zero, so I can see why >> people would be confused. > > We will take care of it, thanks again. > I'm confused. Is Dan's patch correct or is more work needed? If more work is needed, then I prefer to have a single patch correcting the devm_clk_bulk_get_all() handling. Regards, Hans
On 3/7/25 12:58, Hans Verkuil wrote: > On 07/03/2025 10:47, Dmitry Osipenko wrote: >> On 3/7/25 12:45, Dan Carpenter wrote: >>> On Fri, Mar 07, 2025 at 12:36:47PM +0300, Dmitry Osipenko wrote: >>>> On 3/7/25 12:30, Dan Carpenter wrote: >>>>> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>>>> index 4ffc86ad6c35..e0d3fed87a92 100644 >>>>> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>>>> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c >>>>> @@ -154,7 +154,7 @@ struct snps_hdmirx_dev { >>>>> bool hpd_trigger_level_high; >>>>> bool tmds_clk_ratio; >>>>> bool plugged; >>>>> - u32 num_clks; >>>>> + int num_clks; >>>>> u32 edid_blocks_written; >>>>> u32 cur_fmt_fourcc; >>>>> u32 color_depth; >>>> >>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> >>>> Would be also good to return the original error code. There is no need >>>> to check for the < 1 clock, it should be the < 0 check. Can be done in a >>>> separate patch later. Thanks for the fix! >>> >>> I'm not very familiar with th edevm_clk_bulk_get_all() function and it's >>> not documented. But clk_bulk_get_all() does return zero, so I can see why >>> people would be confused. >> >> We will take care of it, thanks again. >> > > I'm confused. Is Dan's patch correct or is more work needed? If more work is > needed, then I prefer to have a single patch correcting the devm_clk_bulk_get_all() > handling. Dan's patch is good, please merge it. Propagation of original error code that I said about will be additional improvement.
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c index 4ffc86ad6c35..e0d3fed87a92 100644 --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c @@ -154,7 +154,7 @@ struct snps_hdmirx_dev { bool hpd_trigger_level_high; bool tmds_clk_ratio; bool plugged; - u32 num_clks; + int num_clks; u32 edid_blocks_written; u32 cur_fmt_fourcc; u32 color_depth;
The num_clks is set this way: hdmirx_dev->num_clks = devm_clk_bulk_get_all(dev, &hdmirx_dev->clks); if (hdmirx_dev->num_clks < 1) return -ENODEV; The devm_clk_bulk_get_all() function returns negative error codes so the hdmirx_dev->num_cks variable needs to be signed for the error handling to work. Fixes: 7b59b132ad43 ("media: platform: synopsys: Add support for HDMI input driver") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)