From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Date: Thu, 7 Apr 2016 23:54:31 +0200 Message-ID: <20160407235431.788dc24a@bbrezillon> References: <1459368249-13241-1-git-send-email-boris.brezillon@free-electrons.com> <1459368249-13241-31-git-send-email-boris.brezillon@free-electrons.com> <20160330212410.GX2350@sirena.org.uk> Reply-To: boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org In-Reply-To: <20160330212410.GX2350-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Mark Brown Cc: Thierry Reding , linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mike Turquette , Stephen Boyd , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Liam Girdwood , Kamil Debski , lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org, Jean Delvare , Guenter Roeck , Dmitry Torokhov , linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Bryan Wu , Richard Purdie , Jacek Anaszewski , linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Joachim Eastwood , Thomas Petazzoni , Heiko Stuebner , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Jingoo Han Lee Jones List-Id: linux-leds@vger.kernel.org Hi Mark, On Wed, 30 Mar 2016 14:24:10 -0700 Mark Brown wrote: > On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote: > > The continuous PWM voltage regulator is caching the voltage value in > > the ->volt_uV field. While most of the time this value should reflect the > > real voltage, sometime it can be sightly different if the PWM device > > rounded the set_duty_cycle request. > > Moreover, this value is not valid until someone has modified the regulator > > output. > > Acked-by: Mark Brown Actually this patch introduces a bug (reported by Stephen): " I applied your patch series [PATCH v5 00/46] pwm: add support for atomic update and found a null pointer dereference when probing a pwm-regulator at boot. See the below stack trace: [ 4.282374] [] pwm_regulator_get_voltage+0x78/0xa0 [ 4.289344] [] regulator_attr_is_visible+0x7c/0x264 [ 4.296408] [] internal_create_group+0x14c/0x280 [ 4.303184] [] sysfs_create_group+0x14/0x1c [ 4.309483] [] sysfs_create_groups+0x30/0x78 [ 4.315881] [] device_add+0x224/0x4d8 [ 4.321609] [] device_register+0x1c/0x28 [ 4.327623] [] regulator_register+0x2e4/0xc14 [ 4.334112] [] devm_regulator_register+0x54/0x94 [ 4.340887] [] pwm_regulator_probe+0x278/0x2b8 [ 4.347473] [] platform_drv_probe+0x58/0xa4 [ 4.353772] [] driver_probe_device+0x114/0x2ac [ 4.360358] [] __driver_attach+0x64/0x90 [ 4.366371] [] bus_for_each_dev+0x74/0x90 [ 4.372478] [] driver_attach+0x20/0x28 [ 4.378299] [] bus_add_driver+0xe8/0x1e0 [ 4.384312] [] driver_register+0x98/0xe4 [ 4.390326] [] __platform_driver_register+0x48/0x50 [ 4.397388] [] pwm_regulator_driver_init+0x18/0x20 [ 4.404356] [] do_one_initcall+0xf8/0x180 [ 4.410466] [] kernel_init_freeable+0x154/0x1f4 [ 4.417148] [] kernel_init+0x10/0xf8 [ 4.422782] [] ret_from_fork+0x10/0x40 It looks like the root cause is that regulator_attr_is_visible will try to get the voltage, but at this point in regulator_register, rdev->constraints is still null. So pwm_duty_cycle_percentage_to_voltage will dereference a null rdev->constraints pointer. " The problem is that we need to know the min and max voltage constraints to calculate the current voltage. ->get_voltage() is called when the sysfs attributes are created (part of device registration), and set_machine_constraints() is called after device_register(), thus leading to the NULL pointer dereference. Is there any reason for calling set_machine_constraints() after device_register() in regulator_register()? Best Regards, Boris From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757777AbcDGVyt (ORCPT ); Thu, 7 Apr 2016 17:54:49 -0400 Received: from down.free-electrons.com ([37.187.137.238]:48812 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752898AbcDGVyq (ORCPT ); Thu, 7 Apr 2016 17:54:46 -0400 Date: Thu, 7 Apr 2016 23:54:31 +0200 From: Boris Brezillon To: Mark Brown Cc: Thierry Reding , linux-pwm@vger.kernel.org, Mike Turquette , Stephen Boyd , linux-clk@vger.kernel.org, Liam Girdwood , Kamil Debski , lm-sensors@lm-sensors.org, Jean Delvare , Guenter Roeck , Dmitry Torokhov , linux-input@vger.kernel.org, Bryan Wu , Richard Purdie , Jacek Anaszewski , linux-leds@vger.kernel.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi@googlegroups.com, Joachim Eastwood , Thomas Petazzoni , Heiko Stuebner , linux-rockchip@lists.infradead.org, Jingoo Han , Lee Jones , linux-fbdev@vger.kernel.org, Jean-Christophe Plagniol-Villard , Tomi Valkeinen , Robert Jarzmik , Alexandre Belloni , Kukjin Kim , Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, intel-gfx@lists.freedesktop.org, Daniel Vetter , Jani Nikula , Jonathan Corbet , linux-doc@vger.kernel.org, David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Hartley Sweeten , Ryan Mallon , Alexander Shiyan , Milo Kim , Stephen Barber Subject: Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Message-ID: <20160407235431.788dc24a@bbrezillon> In-Reply-To: <20160330212410.GX2350@sirena.org.uk> References: <1459368249-13241-1-git-send-email-boris.brezillon@free-electrons.com> <1459368249-13241-31-git-send-email-boris.brezillon@free-electrons.com> <20160330212410.GX2350@sirena.org.uk> X-Mailer: Claws Mail 3.12.0 (GTK+ 2.24.28; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mark, On Wed, 30 Mar 2016 14:24:10 -0700 Mark Brown wrote: > On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote: > > The continuous PWM voltage regulator is caching the voltage value in > > the ->volt_uV field. While most of the time this value should reflect the > > real voltage, sometime it can be sightly different if the PWM device > > rounded the set_duty_cycle request. > > Moreover, this value is not valid until someone has modified the regulator > > output. > > Acked-by: Mark Brown Actually this patch introduces a bug (reported by Stephen): " I applied your patch series [PATCH v5 00/46] pwm: add support for atomic update and found a null pointer dereference when probing a pwm-regulator at boot. See the below stack trace: [ 4.282374] [] pwm_regulator_get_voltage+0x78/0xa0 [ 4.289344] [] regulator_attr_is_visible+0x7c/0x264 [ 4.296408] [] internal_create_group+0x14c/0x280 [ 4.303184] [] sysfs_create_group+0x14/0x1c [ 4.309483] [] sysfs_create_groups+0x30/0x78 [ 4.315881] [] device_add+0x224/0x4d8 [ 4.321609] [] device_register+0x1c/0x28 [ 4.327623] [] regulator_register+0x2e4/0xc14 [ 4.334112] [] devm_regulator_register+0x54/0x94 [ 4.340887] [] pwm_regulator_probe+0x278/0x2b8 [ 4.347473] [] platform_drv_probe+0x58/0xa4 [ 4.353772] [] driver_probe_device+0x114/0x2ac [ 4.360358] [] __driver_attach+0x64/0x90 [ 4.366371] [] bus_for_each_dev+0x74/0x90 [ 4.372478] [] driver_attach+0x20/0x28 [ 4.378299] [] bus_add_driver+0xe8/0x1e0 [ 4.384312] [] driver_register+0x98/0xe4 [ 4.390326] [] __platform_driver_register+0x48/0x50 [ 4.397388] [] pwm_regulator_driver_init+0x18/0x20 [ 4.404356] [] do_one_initcall+0xf8/0x180 [ 4.410466] [] kernel_init_freeable+0x154/0x1f4 [ 4.417148] [] kernel_init+0x10/0xf8 [ 4.422782] [] ret_from_fork+0x10/0x40 It looks like the root cause is that regulator_attr_is_visible will try to get the voltage, but at this point in regulator_register, rdev->constraints is still null. So pwm_duty_cycle_percentage_to_voltage will dereference a null rdev->constraints pointer. " The problem is that we need to know the min and max voltage constraints to calculate the current voltage. ->get_voltage() is called when the sysfs attributes are created (part of device registration), and set_machine_constraints() is called after device_register(), thus leading to the NULL pointer dereference. Is there any reason for calling set_machine_constraints() after device_register() in regulator_register()? Best Regards, Boris From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Thu, 07 Apr 2016 21:54:31 +0000 Subject: Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Message-Id: <20160407235431.788dc24a@bbrezillon> List-Id: References: <1459368249-13241-1-git-send-email-boris.brezillon@free-electrons.com> <1459368249-13241-31-git-send-email-boris.brezillon@free-electrons.com> <20160330212410.GX2350@sirena.org.uk> In-Reply-To: <20160330212410.GX2350-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Mark Brown Cc: Thierry Reding , linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mike Turquette , Stephen Boyd , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Liam Girdwood , Kamil Debski , lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org, Jean Delvare , Guenter Roeck , Dmitry Torokhov , linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Bryan Wu , Richard Purdie , Jacek Anaszewski , linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Joachim Eastwood , Thomas Petazzoni , Heiko Stuebner , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Jingoo Han Hi Mark, On Wed, 30 Mar 2016 14:24:10 -0700 Mark Brown wrote: > On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote: > > The continuous PWM voltage regulator is caching the voltage value in > > the ->volt_uV field. While most of the time this value should reflect the > > real voltage, sometime it can be sightly different if the PWM device > > rounded the set_duty_cycle request. > > Moreover, this value is not valid until someone has modified the regulator > > output. > > Acked-by: Mark Brown Actually this patch introduces a bug (reported by Stephen): " I applied your patch series [PATCH v5 00/46] pwm: add support for atomic update and found a null pointer dereference when probing a pwm-regulator at boot. See the below stack trace: [ 4.282374] [] pwm_regulator_get_voltage+0x78/0xa0 [ 4.289344] [] regulator_attr_is_visible+0x7c/0x264 [ 4.296408] [] internal_create_group+0x14c/0x280 [ 4.303184] [] sysfs_create_group+0x14/0x1c [ 4.309483] [] sysfs_create_groups+0x30/0x78 [ 4.315881] [] device_add+0x224/0x4d8 [ 4.321609] [] device_register+0x1c/0x28 [ 4.327623] [] regulator_register+0x2e4/0xc14 [ 4.334112] [] devm_regulator_register+0x54/0x94 [ 4.340887] [] pwm_regulator_probe+0x278/0x2b8 [ 4.347473] [] platform_drv_probe+0x58/0xa4 [ 4.353772] [] driver_probe_device+0x114/0x2ac [ 4.360358] [] __driver_attach+0x64/0x90 [ 4.366371] [] bus_for_each_dev+0x74/0x90 [ 4.372478] [] driver_attach+0x20/0x28 [ 4.378299] [] bus_add_driver+0xe8/0x1e0 [ 4.384312] [] driver_register+0x98/0xe4 [ 4.390326] [] __platform_driver_register+0x48/0x50 [ 4.397388] [] pwm_regulator_driver_init+0x18/0x20 [ 4.404356] [] do_one_initcall+0xf8/0x180 [ 4.410466] [] kernel_init_freeable+0x154/0x1f4 [ 4.417148] [] kernel_init+0x10/0xf8 [ 4.422782] [] ret_from_fork+0x10/0x40 It looks like the root cause is that regulator_attr_is_visible will try to get the voltage, but at this point in regulator_register, rdev->constraints is still null. So pwm_duty_cycle_percentage_to_voltage will dereference a null rdev->constraints pointer. " The problem is that we need to know the min and max voltage constraints to calculate the current voltage. ->get_voltage() is called when the sysfs attributes are created (part of device registration), and set_machine_constraints() is called after device_register(), thus leading to the NULL pointer dereference. Is there any reason for calling set_machine_constraints() after device_register() in regulator_register()? Best Regards, Boris From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@free-electrons.com (Boris Brezillon) Date: Thu, 7 Apr 2016 23:54:31 +0200 Subject: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage In-Reply-To: <20160330212410.GX2350@sirena.org.uk> References: <1459368249-13241-1-git-send-email-boris.brezillon@free-electrons.com> <1459368249-13241-31-git-send-email-boris.brezillon@free-electrons.com> <20160330212410.GX2350@sirena.org.uk> Message-ID: <20160407235431.788dc24a@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Mark, On Wed, 30 Mar 2016 14:24:10 -0700 Mark Brown wrote: > On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote: > > The continuous PWM voltage regulator is caching the voltage value in > > the ->volt_uV field. While most of the time this value should reflect the > > real voltage, sometime it can be sightly different if the PWM device > > rounded the set_duty_cycle request. > > Moreover, this value is not valid until someone has modified the regulator > > output. > > Acked-by: Mark Brown Actually this patch introduces a bug (reported by Stephen): " I applied your patch series [PATCH v5 00/46] pwm: add support for atomic update and found a null pointer dereference when probing a pwm-regulator at boot. See the below stack trace: [ 4.282374] [] pwm_regulator_get_voltage+0x78/0xa0 [ 4.289344] [] regulator_attr_is_visible+0x7c/0x264 [ 4.296408] [] internal_create_group+0x14c/0x280 [ 4.303184] [] sysfs_create_group+0x14/0x1c [ 4.309483] [] sysfs_create_groups+0x30/0x78 [ 4.315881] [] device_add+0x224/0x4d8 [ 4.321609] [] device_register+0x1c/0x28 [ 4.327623] [] regulator_register+0x2e4/0xc14 [ 4.334112] [] devm_regulator_register+0x54/0x94 [ 4.340887] [] pwm_regulator_probe+0x278/0x2b8 [ 4.347473] [] platform_drv_probe+0x58/0xa4 [ 4.353772] [] driver_probe_device+0x114/0x2ac [ 4.360358] [] __driver_attach+0x64/0x90 [ 4.366371] [] bus_for_each_dev+0x74/0x90 [ 4.372478] [] driver_attach+0x20/0x28 [ 4.378299] [] bus_add_driver+0xe8/0x1e0 [ 4.384312] [] driver_register+0x98/0xe4 [ 4.390326] [] __platform_driver_register+0x48/0x50 [ 4.397388] [] pwm_regulator_driver_init+0x18/0x20 [ 4.404356] [] do_one_initcall+0xf8/0x180 [ 4.410466] [] kernel_init_freeable+0x154/0x1f4 [ 4.417148] [] kernel_init+0x10/0xf8 [ 4.422782] [] ret_from_fork+0x10/0x40 It looks like the root cause is that regulator_attr_is_visible will try to get the voltage, but at this point in regulator_register, rdev->constraints is still null. So pwm_duty_cycle_percentage_to_voltage will dereference a null rdev->constraints pointer. " The problem is that we need to know the min and max voltage constraints to calculate the current voltage. ->get_voltage() is called when the sysfs attributes are created (part of device registration), and set_machine_constraints() is called after device_register(), thus leading to the NULL pointer dereference. Is there any reason for calling set_machine_constraints() after device_register() in regulator_register()? Best Regards, Boris