diff mbox series

cpufreq: exit() callback is optional

Message ID b97964653d02225f061e0c2a650b365c354b98c8.1712900945.git.viresh.kumar@linaro.org
State New
Headers show
Series cpufreq: exit() callback is optional | expand

Commit Message

Viresh Kumar April 12, 2024, 5:49 a.m. UTC
The exit() callback is optional and shouldn't be called without checking
a valid pointer first.

Also, we must clear freq_table pointer even if the exit() callback isn't
present.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Viresh Kumar April 12, 2024, 6:27 a.m. UTC | #1
On 12-04-24, 14:19, lizhe wrote:
> Why did you do that? Why did you plagiarize others' achievements? Is this the style of Linaro?

Even if your changes make sense, the discussions needs to be healthy. I am a
co-maintainer of the cpufreq subsystem and its mine and Rafael's responsibility
to keep things moving in the right direction.

This patch fixes a issue you never mentioned over LKML.

Lets not make this awkward, it won't help anyone.

Thanks.
Viresh Kumar April 12, 2024, 6:32 a.m. UTC | #2
Getting the Cc list back, + Greg.

Greg,

Looks like another one of those experiments with the community ?

:)

On 12-04-24, 14:27, lizhe wrote:
> You are really disgusting and have no manners at all. This makes people feel disgusted with your company.
> 
> 
> 
> ---- Replied Message ----
> | From | Viresh Kumar<viresh.kumar@linaro.org> |
> | Date | 04/12/2024 14:24 |
> | To | lizhe<sensor1010@163.com> |
> | Cc | rafael<rafael@kernel.org>、linux-pm<linux-pm@vger.kernel.org>、Vincent Guittot<vincent.guittot@linaro.org>、linux-kernel<linux-kernel@vger.kernel.org> |
> | Subject | Re: [PATCH] cpufreq: exit() callback is optional |
> On 12-04-24, 14:12, lizhe wrote:
> > I was the first one to find this problem, so the patch should be submitted by me.
> 
> :)
> 
> This patch doesn't take away any of the work you have done. What you are trying
> to do is simplify drivers with empty exit callback and the unused return value
> of the callback.
> 
> And what I am trying to do is fix a bug in the cpufreq core, which only makes
> your other patches more acceptable.
> 
> So no, you never identified the problem this patch is trying to solve.
> 
> Please don't feel that anyone is trying to take away your hardwork. That's not
> how things are done here. We appreciate anyone who is spending time to make the
> kernel better.
> 
> If I were to take credit of your work, then I would have sent a big patch to fix
> the exit() callback issue you are trying to solve, with randomly sent patches.
Viresh Kumar April 12, 2024, 9:21 a.m. UTC | #3
On 12-04-24, 17:05, lizhe wrote:
> I found it first and submitted it to the main line first. Please be fair and just. Let him withdraw his patch.

Well, I decided not to reply to your emails anymore but this needs to
be clarified a bit now.

You sent a lot of patches, over and over again and it was a mess. I
saw the this [1] series first and went over to read the code and fixed
an issue which I found (by the $subject patch).

Later I read your other patch [2], which I Acked roughly two hours
back and yes you did send a patch that fixed the problem partially. I
never saw it earlier, which is okay and it happens. Despite me giving
an Ack to your patch, you have sent half-a-dozen more emails..

Then I posted a newer version of my patch some time back, removing the
bits you already fixed [3].

That is all one side of the story. But all the noise you have created
here has really demotivated people to review your stuff now.

--
Viresh

[1] https://lore.kernel.org/all/20240410132132.3526-1-sensor1010@163.com/
[2] https://lore.kernel.org/all/20240411231818.2471-1-sensor1010@163.com/
[3] https://lore.kernel.org/all/68294ce534668c6ab3b71a1b3e6650227c6e1f20.1712911186.git.viresh.kumar@linaro.org/
Rafael J. Wysocki April 12, 2024, 11:08 a.m. UTC | #4
On Fri, Apr 12, 2024 at 7:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The exit() callback is optional and shouldn't be called without checking
> a valid pointer first.
>
> Also, we must clear freq_table pointer even if the exit() callback isn't
> present.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 66e10a19d76a..fd9c3ed21f49 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1679,10 +1679,13 @@ static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy)
>          */
>         if (cpufreq_driver->offline) {
>                 cpufreq_driver->offline(policy);
> -       } else if (cpufreq_driver->exit) {
> -               cpufreq_driver->exit(policy);
> -               policy->freq_table = NULL;
> +               return;
>         }
> +
> +       if (cpufreq_driver->exit)
> +               cpufreq_driver->exit(policy);
> +
> +       policy->freq_table = NULL;
>  }
>
>  static int cpufreq_offline(unsigned int cpu)
> @@ -1740,7 +1743,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>         }
>
>         /* We did light-weight exit earlier, do full tear down now */
> -       if (cpufreq_driver->offline)
> +       if (cpufreq_driver->offline && cpufreq_driver->exit)
>                 cpufreq_driver->exit(policy);
>
>         up_write(&policy->rwsem);
> --

I have applied this patch (for 6.10 because I don't think it is
urgent) because it addresses both issues with missing ->exit() driver
callback checks.  I honestly don't think it would be better to apply a
separate patch for each of them.

Thanks!
Lizhe April 12, 2024, 2:31 p.m. UTC | #5
HI


i will review how it all started.
1. 
  i  reported the vulnerability to the maintainer.
   


2. manitaner replay me


3.   i report to main line 
     


4.  you reply me 
     


5 .  you report to main line 
      




You submitted the patch to the mainline after you learned about the vulnerability from the patch I submitted




    



At 2024-04-12 21:54:00, "lizhe" <sensor1010@163.com> wrote:




Hi 

     Viresh Kumar
     please add my name to the signature. because i discovered this vulnerability. 
     please reply me.
     thanks




   

 


At 2024-04-12 18:28:56, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Fri, Apr 12, 2024 at 02:51:08PM +0530, Viresh Kumar wrote:
>> On 12-04-24, 17:05, lizhe wrote:
>> > I found it first and submitted it to the main line first. Please be fair and just. Let him withdraw his patch.
>> 
>> Well, I decided not to reply to your emails anymore but this needs to
>> be clarified a bit now.
>> 
>> You sent a lot of patches, over and over again and it was a mess. I
>> saw the this [1] series first and went over to read the code and fixed
>> an issue which I found (by the $subject patch).
>> 
>> Later I read your other patch [2], which I Acked roughly two hours
>> back and yes you did send a patch that fixed the problem partially. I
>> never saw it earlier, which is okay and it happens. Despite me giving
>> an Ack to your patch, you have sent half-a-dozen more emails..
>> 
>> Then I posted a newer version of my patch some time back, removing the
>> bits you already fixed [3].
>> 
>> That is all one side of the story. But all the noise you have created
>> here has really demotivated people to review your stuff now.
>> 
>> --
>> Viresh
>> 
>> [1] https://lore.kernel.org/all/20240410132132.3526-1-sensor1010@163.com/
>> [2] https://lore.kernel.org/all/20240411231818.2471-1-sensor1010@163.com/
>> [3] https://lore.kernel.org/all/68294ce534668c6ab3b71a1b3e6650227c6e1f20.1712911186.git.viresh.kumar@linaro.org/
>
>Thanks for the links, I don't see that you did anything wrong here at
>all.
>
>Lizhe, you seem to be confused as to how kernel development works.  I
>suggest you take some time off and read up on how this all is supposed
>to happen and then work with some local people, in person, to get this
>figured out first, before submitting changes again.
>
>thanks,
>
>greg k-h
shuah April 22, 2024, 11:21 a.m. UTC | #6
Hi Lizhe,

On 4/12/24 08:41, lizhe wrote:
> 
> Let's both take a step back and add my signature to the patch,
> since I was the one who discovered and reported the vulnerability.
> 
> 

You might have discovered the vulnerability and sent in a patch. After that,
it is totally up to the maintainer to decide to accept or reject the patch.
Developers can't demand their patches to be reviewed and/or accepted. They
can request a review and inclusion if maintainer deems it acceptable.

In this email thread, I can see that maintainers and developers have been
advising you to understand the kernel development process.

Refer to the following document to understand the process:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst

Refer to the following Contributor Covenant Code of Conduct to understand the
code of conduct the Linux kernel community abides by:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/code-of-conduct.rst

thanks,
-- Shuah (On behalf of the Code of Conduct Committee)
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 66e10a19d76a..fd9c3ed21f49 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1679,10 +1679,13 @@  static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy)
 	 */
 	if (cpufreq_driver->offline) {
 		cpufreq_driver->offline(policy);
-	} else if (cpufreq_driver->exit) {
-		cpufreq_driver->exit(policy);
-		policy->freq_table = NULL;
+		return;
 	}
+
+	if (cpufreq_driver->exit)
+		cpufreq_driver->exit(policy);
+
+	policy->freq_table = NULL;
 }
 
 static int cpufreq_offline(unsigned int cpu)
@@ -1740,7 +1743,7 @@  static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	}
 
 	/* We did light-weight exit earlier, do full tear down now */
-	if (cpufreq_driver->offline)
+	if (cpufreq_driver->offline && cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 
 	up_write(&policy->rwsem);