[bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Dan Carpenter]

Hello Sean Paul,

The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
well" from Jul 25, 2018, leads to the following
Smatch static checker warning:

	drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
	warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')

drivers/gpu/drm/msm/dsi/dsi_host.c
    721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
    722 {
    723         if (!msm_host->mode) {
    724                 pr_err("%s: mode not set\n", __func__);
    725                 return -EINVAL;
    726         }
    727 
    728         dsi_calc_pclk(msm_host, is_bonded_dsi);
--> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
                ^^^^^^^^^^^^^^^^^^^^^^
I don't know why Smatch is suddenly warning about ancient msm code, but
clock rates should be unsigned long.  (I don't remember why).

    730         return 0;
    731 }

regards,
dan carpenter

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[jesszhan]

Hey Dan,

Thanks for the report, will take care of it.

On 2021-10-01 05:31, Dan Carpenter wrote:
> Hello Sean Paul,
> 
> The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
> well" from Jul 25, 2018, leads to the following
> Smatch static checker warning:
> 
> 	drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
> 	warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
> 
> drivers/gpu/drm/msm/dsi/dsi_host.c
>     721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool
> is_bonded_dsi)
>     722 {
>     723         if (!msm_host->mode) {
>     724                 pr_err("%s: mode not set\n", __func__);
>     725                 return -EINVAL;
>     726         }
>     727
>     728         dsi_calc_pclk(msm_host, is_bonded_dsi);
> --> 729         msm_host->esc_clk_rate = 
> clk_get_rate(msm_host->esc_clk);
>                 ^^^^^^^^^^^^^^^^^^^^^^
> I don't know why Smatch is suddenly warning about ancient msm code, but
> clock rates should be unsigned long.  (I don't remember why).
> 
>     730         return 0;
>     731 }
> 
> regards,
> dan carpenter

Thanks,
Jessica Zhang

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Jessica Zhang]

Hey Dan,

On 10/1/2021 5:31 AM, Dan Carpenter wrote:
> Hello Sean Paul,
>
> The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
> well" from Jul 25, 2018, leads to the following
> Smatch static checker warning:
>
> 	drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
> 	warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
>
> drivers/gpu/drm/msm/dsi/dsi_host.c
>      721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>      722 {
>      723         if (!msm_host->mode) {
>      724                 pr_err("%s: mode not set\n", __func__);
>      725                 return -EINVAL;
>      726         }
>      727
>      728         dsi_calc_pclk(msm_host, is_bonded_dsi);
> --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>                  ^^^^^^^^^^^^^^^^^^^^^^
> I don't know why Smatch is suddenly warning about ancient msm code, but
> clock rates should be unsigned long.  (I don't remember why).
>
>      730         return 0;
>      731 }

I'm unable to recreate the warning with Smatch. After running 
build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker 
drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:

CHECK scripts/mod/empty.c
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CHECK arch/arm64/kernel/vdso/vgettimeofday.c
CC drivers/gpu/drm/msm/dsi/dsi_host.o
CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn: 
missing error code 'ret'

Is there a specific .config you're using (that's not the default 
mainline defconfig)? If so, can you please share it?

Thanks,

Jessica Zhang

> regards,
> dan carpenter

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Dan Carpenter]

On Thu, Oct 14, 2021 at 06:43:22PM -0700, Jessica Zhang wrote:
> Hey Dan,
> 
> On 10/1/2021 5:31 AM, Dan Carpenter wrote:
> > Hello Sean Paul,
> > 
> > The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
> > well" from Jul 25, 2018, leads to the following
> > Smatch static checker warning:
> > 
> > 	drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
> > 	warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
> > 
> > drivers/gpu/drm/msm/dsi/dsi_host.c
> >      721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >      722 {
> >      723         if (!msm_host->mode) {
> >      724                 pr_err("%s: mode not set\n", __func__);
> >      725                 return -EINVAL;
> >      726         }
> >      727
> >      728         dsi_calc_pclk(msm_host, is_bonded_dsi);
> > --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
> >                  ^^^^^^^^^^^^^^^^^^^^^^
> > I don't know why Smatch is suddenly warning about ancient msm code, but
> > clock rates should be unsigned long.  (I don't remember why).
> > 
> >      730         return 0;
> >      731 }
> 
> I'm unable to recreate the warning with Smatch. After running
> build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker
> drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:
> 
> CHECK scripts/mod/empty.c
> CALL scripts/checksyscalls.sh
> CALL scripts/atomic/check-atomics.sh
> CHECK arch/arm64/kernel/vdso/vgettimeofday.c
> CC drivers/gpu/drm/msm/dsi/dsi_host.o
> CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
> drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn:
> missing error code 'ret'
> 
> Is there a specific .config you're using (that's not the default mainline
> defconfig)? If so, can you please share it?

Oh, sorry.  I never published this Smatch check.  It generates 236
warnings and I'm not sure the rules here about where clk has to be
unsigned long so I can't publish it...  I think someone told me that it
has to be unsigned long?

regards,
dan carpenter

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Jessica Zhang]

On 10/15/2021 1:12 AM, Dan Carpenter wrote:
> On Thu, Oct 14, 2021 at 06:43:22PM -0700, Jessica Zhang wrote:
>> Hey Dan,
>>
>> On 10/1/2021 5:31 AM, Dan Carpenter wrote:
>>> Hello Sean Paul,
>>>
>>> The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
>>> well" from Jul 25, 2018, leads to the following
>>> Smatch static checker warning:
>>>
>>> 	drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
>>> 	warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
>>>
>>> drivers/gpu/drm/msm/dsi/dsi_host.c
>>>       721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>       722 {
>>>       723         if (!msm_host->mode) {
>>>       724                 pr_err("%s: mode not set\n", __func__);
>>>       725                 return -EINVAL;
>>>       726         }
>>>       727
>>>       728         dsi_calc_pclk(msm_host, is_bonded_dsi);
>>> --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>>                   ^^^^^^^^^^^^^^^^^^^^^^
>>> I don't know why Smatch is suddenly warning about ancient msm code, but
>>> clock rates should be unsigned long.  (I don't remember why).
>>>
>>>       730         return 0;
>>>       731 }
>> I'm unable to recreate the warning with Smatch. After running
>> build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker
>> drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:
>>
>> CHECK scripts/mod/empty.c
>> CALL scripts/checksyscalls.sh
>> CALL scripts/atomic/check-atomics.sh
>> CHECK arch/arm64/kernel/vdso/vgettimeofday.c
>> CC drivers/gpu/drm/msm/dsi/dsi_host.o
>> CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
>> drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn:
>> missing error code 'ret'
>>
>> Is there a specific .config you're using (that's not the default mainline
>> defconfig)? If so, can you please share it?
> Oh, sorry.  I never published this Smatch check.  It generates 236
> warnings and I'm not sure the rules here about where clk has to be
> unsigned long so I can't publish it...  I think someone told me that it
> has to be unsigned long?

Can you share which Smatch script (+ any command line options) you used 
to generate this warning? Just want to make sure I'm able to properly 
recreate this warning with Smatch on my end.

Thanks,

Jessica Zhang

> regards,
> dan carpenter
>

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Dmitry Baryshkov]

On Fri, 15 Oct 2021 at 04:43, Jessica Zhang <jesszhan@codeaurora.org> wrote:
>
> Hey Dan,
>
> On 10/1/2021 5:31 AM, Dan Carpenter wrote:
> > Hello Sean Paul,
> >
> > The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
> > well" from Jul 25, 2018, leads to the following
> > Smatch static checker warning:
> >
> >       drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
> >       warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
> >
> > drivers/gpu/drm/msm/dsi/dsi_host.c
> >      721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >      722 {
> >      723         if (!msm_host->mode) {
> >      724                 pr_err("%s: mode not set\n", __func__);
> >      725                 return -EINVAL;
> >      726         }
> >      727
> >      728         dsi_calc_pclk(msm_host, is_bonded_dsi);
> > --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
> >                  ^^^^^^^^^^^^^^^^^^^^^^
> > I don't know why Smatch is suddenly warning about ancient msm code, but
> > clock rates should be unsigned long.  (I don't remember why).
> >
> >      730         return 0;
> >      731 }
>
> I'm unable to recreate the warning with Smatch. After running
> build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker
> drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:
>
> CHECK scripts/mod/empty.c
> CALL scripts/checksyscalls.sh
> CALL scripts/atomic/check-atomics.sh
> CHECK arch/arm64/kernel/vdso/vgettimeofday.c
> CC drivers/gpu/drm/msm/dsi/dsi_host.o
> CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
> drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn:
> missing error code 'ret'
>
> Is there a specific .config you're using (that's not the default
> mainline defconfig)? If so, can you please share it?

Are you running your checks with ARM32 or ARM64 in mind?
Note, esc_clk_rate is u32, while clk_get_rate()'s returns unsigned long.
It would make sense to change all three clocks rates in msm_dsi_host
struct (and several places where they are used) to unsigned long.

>
> Thanks,
>
> Jessica Zhang
>
> > regards,
> > dan carpenter



-- 
With best wishes
Dmitry

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Jessica Zhang]

Hey Dmitry,

On 10/15/2021 11:24 AM, Dmitry Baryshkov wrote:
> On Fri, 15 Oct 2021 at 04:43, Jessica Zhang <jesszhan@codeaurora.org> wrote:
>> Hey Dan,
>>
>> On 10/1/2021 5:31 AM, Dan Carpenter wrote:
>>> Hello Sean Paul,
>>>
>>> The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
>>> well" from Jul 25, 2018, leads to the following
>>> Smatch static checker warning:
>>>
>>>        drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
>>>        warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
>>>
>>> drivers/gpu/drm/msm/dsi/dsi_host.c
>>>       721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>       722 {
>>>       723         if (!msm_host->mode) {
>>>       724                 pr_err("%s: mode not set\n", __func__);
>>>       725                 return -EINVAL;
>>>       726         }
>>>       727
>>>       728         dsi_calc_pclk(msm_host, is_bonded_dsi);
>>> --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>>                   ^^^^^^^^^^^^^^^^^^^^^^
>>> I don't know why Smatch is suddenly warning about ancient msm code, but
>>> clock rates should be unsigned long.  (I don't remember why).
>>>
>>>       730         return 0;
>>>       731 }
>> I'm unable to recreate the warning with Smatch. After running
>> build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker
>> drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:
>>
>> CHECK scripts/mod/empty.c
>> CALL scripts/checksyscalls.sh
>> CALL scripts/atomic/check-atomics.sh
>> CHECK arch/arm64/kernel/vdso/vgettimeofday.c
>> CC drivers/gpu/drm/msm/dsi/dsi_host.o
>> CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
>> drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn:
>> missing error code 'ret'
>>
>> Is there a specific .config you're using (that's not the default
>> mainline defconfig)? If so, can you please share it?
> Are you running your checks with ARM32 or ARM64 in mind?
> Note, esc_clk_rate is u32, while clk_get_rate()'s returns unsigned long.
> It would make sense to change all three clocks rates in msm_dsi_host
> struct (and several places where they are used) to unsigned long.

Thanks for the response. I'm aware of what's causing this issue and how 
to fix it, but I want to also be able to recreate the warning locally 
with Smatch.

Thanks,

Jessica Zhang

>> Thanks,
>>
>> Jessica Zhang
>>
>>> regards,
>>> dan carpenter
>
>

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Dan Carpenter]

On Fri, Oct 15, 2021 at 12:34:20PM -0700, Jessica Zhang wrote:
> Hey Dmitry,
> 
> On 10/15/2021 11:24 AM, Dmitry Baryshkov wrote:
> > On Fri, 15 Oct 2021 at 04:43, Jessica Zhang <jesszhan@codeaurora.org> wrote:
> > > Hey Dan,
> > > 
> > > On 10/1/2021 5:31 AM, Dan Carpenter wrote:
> > > > Hello Sean Paul,
> > > > 
> > > > The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
> > > > well" from Jul 25, 2018, leads to the following
> > > > Smatch static checker warning:
> > > > 
> > > >        drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
> > > >        warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
> > > > 
> > > > drivers/gpu/drm/msm/dsi/dsi_host.c
> > > >       721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > >       722 {
> > > >       723         if (!msm_host->mode) {
> > > >       724                 pr_err("%s: mode not set\n", __func__);
> > > >       725                 return -EINVAL;
> > > >       726         }
> > > >       727
> > > >       728         dsi_calc_pclk(msm_host, is_bonded_dsi);
> > > > --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
> > > >                   ^^^^^^^^^^^^^^^^^^^^^^
> > > > I don't know why Smatch is suddenly warning about ancient msm code, but
> > > > clock rates should be unsigned long.  (I don't remember why).
> > > > 
> > > >       730         return 0;
> > > >       731 }
> > > I'm unable to recreate the warning with Smatch. After running
> > > build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker
> > > drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:
> > > 
> > > CHECK scripts/mod/empty.c
> > > CALL scripts/checksyscalls.sh
> > > CALL scripts/atomic/check-atomics.sh
> > > CHECK arch/arm64/kernel/vdso/vgettimeofday.c
> > > CC drivers/gpu/drm/msm/dsi/dsi_host.o
> > > CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
> > > drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn:
> > > missing error code 'ret'
> > > 
> > > Is there a specific .config you're using (that's not the default
> > > mainline defconfig)? If so, can you please share it?
> > Are you running your checks with ARM32 or ARM64 in mind?
> > Note, esc_clk_rate is u32, while clk_get_rate()'s returns unsigned long.
> > It would make sense to change all three clocks rates in msm_dsi_host
> > struct (and several places where they are used) to unsigned long.
> 
> Thanks for the response. I'm aware of what's causing this issue and how to
> fix it, but I want to also be able to recreate the warning locally with
> Smatch.

No, sorry, I haven't published that check.  It's just something I have
locally.

Btw, I will be offline for the next two weeks...

regards,
dan carpenter

Re: [bug report] drm/msm: dsi: Handle dual-channel for 6G as well

[Jessica Zhang]

On 10/16/2021 12:35 PM, Dan Carpenter wrote:
> On Fri, Oct 15, 2021 at 12:34:20PM -0700, Jessica Zhang wrote:
>> Hey Dmitry,
>>
>> On 10/15/2021 11:24 AM, Dmitry Baryshkov wrote:
>>> On Fri, 15 Oct 2021 at 04:43, Jessica Zhang <jesszhan@codeaurora.org> wrote:
>>>> Hey Dan,
>>>>
>>>> On 10/1/2021 5:31 AM, Dan Carpenter wrote:
>>>>> Hello Sean Paul,
>>>>>
>>>>> The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as
>>>>> well" from Jul 25, 2018, leads to the following
>>>>> Smatch static checker warning:
>>>>>
>>>>>         drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g()
>>>>>         warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong')
>>>>>
>>>>> drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>        721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>        722 {
>>>>>        723         if (!msm_host->mode) {
>>>>>        724                 pr_err("%s: mode not set\n", __func__);
>>>>>        725                 return -EINVAL;
>>>>>        726         }
>>>>>        727
>>>>>        728         dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>> --> 729         msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>>>>                    ^^^^^^^^^^^^^^^^^^^^^^
>>>>> I don't know why Smatch is suddenly warning about ancient msm code, but
>>>>> clock rates should be unsigned long.  (I don't remember why).
>>>>>
>>>>>        730         return 0;
>>>>>        731 }
>>>> I'm unable to recreate the warning with Smatch. After running
>>>> build_kernel_data.sh, I ran `<path to smatch>/smatch_scripts/kchecker
>>>> drivers/gpu/drm/msm/dsi/dsi_host.c` and got the following output:
>>>>
>>>> CHECK scripts/mod/empty.c
>>>> CALL scripts/checksyscalls.sh
>>>> CALL scripts/atomic/check-atomics.sh
>>>> CHECK arch/arm64/kernel/vdso/vgettimeofday.c
>>>> CC drivers/gpu/drm/msm/dsi/dsi_host.o
>>>> CHECK drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> drivers/gpu/drm/msm/dsi/dsi_host.c:2380 msm_dsi_host_power_on() warn:
>>>> missing error code 'ret'
>>>>
>>>> Is there a specific .config you're using (that's not the default
>>>> mainline defconfig)? If so, can you please share it?
>>> Are you running your checks with ARM32 or ARM64 in mind?
>>> Note, esc_clk_rate is u32, while clk_get_rate()'s returns unsigned long.
>>> It would make sense to change all three clocks rates in msm_dsi_host
>>> struct (and several places where they are used) to unsigned long.
>> Thanks for the response. I'm aware of what's causing this issue and how to
>> fix it, but I want to also be able to recreate the warning locally with
>> Smatch.
> No, sorry, I haven't published that check.  It's just something I have
> locally.
Understood. It would be helpful for future warnings if the issue was 
reproducible using Smatch scripts available in the repo since we would 
be able to verify the fix locally.
>
> Btw, I will be offline for the next two weeks...

Gotcha, thanks for the heads up! I will release a fix ASAP for you to ack.

Best,

Jessica Zhang

> regards,
> dan carpenter
>
>