| Message ID | 1359121471-21457-2-git-send-email-florian.vaussard@epfl.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/25/2013 02:44 PM, Florian Vaussard wrote: > Calls to some external PWM chips can sleep. To help users, > add pwm_cansleep() API. > > Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > drivers/pwm/core.c | 12 ++++++++++++ > include/linux/pwm.h | 10 ++++++++++ > 2 files changed, 22 insertions(+), 0 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 4a13da4..e737f5f 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm) > } > EXPORT_SYMBOL_GPL(devm_pwm_put); > > +/** > + * pwm_cansleep() - report whether pwm access will sleep > + * @pwm: PWM device > + * > + * It returns nonzero if accessing the PWM can sleep. > + */ > +int pwm_cansleep(struct pwm_device *pwm) > +{ > + return pwm->chip->can_sleep; > +} > +EXPORT_SYMBOL_GPL(pwm_cansleep); > + > #ifdef CONFIG_DEBUG_FS > static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) > { > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 70655a2..e2cb5c7 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -146,6 +146,8 @@ struct pwm_ops { > * @base: number of first PWM controlled by this chip > * @npwm: number of PWMs controlled by this chip > * @pwms: array of PWM devices allocated by the framework > + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep, > + * as they must while accessing PWM chips over I2C or SPI > */ > struct pwm_chip { > struct device *dev; > @@ -159,6 +161,7 @@ struct pwm_chip { > struct pwm_device * (*of_xlate)(struct pwm_chip *pc, > const struct of_phandle_args *args); > unsigned int of_pwm_n_cells; > + unsigned int can_sleep:1; > }; > > #if IS_ENABLED(CONFIG_PWM) > @@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id); > struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np, > const char *con_id); > void devm_pwm_put(struct device *dev, struct pwm_device *pwm); > + > +int pwm_cansleep(struct pwm_device *pwm); > #else > static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data) > { > @@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev, > static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm) > { > } > + > +static inline int pwm_cansleep(struct pwm_device *pwm) > +{ > + return 0; > +} > #endif > > struct pwm_lookup { >
On Fri, Jan 25, 2013 at 02:44:29PM +0100, Florian Vaussard wrote: > Calls to some external PWM chips can sleep. To help users, > add pwm_cansleep() API. > > Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> > --- > drivers/pwm/core.c | 12 ++++++++++++ > include/linux/pwm.h | 10 ++++++++++ > 2 files changed, 22 insertions(+), 0 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 4a13da4..e737f5f 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm) > } > EXPORT_SYMBOL_GPL(devm_pwm_put); > > +/** > + * pwm_cansleep() - report whether pwm access will sleep "... whether PWM access..." please. > + * @pwm: PWM device > + * > + * It returns nonzero if accessing the PWM can sleep. > + */ > +int pwm_cansleep(struct pwm_device *pwm) I actually liked pwm_can_sleep() better. I find it to be more consistent with the naming of other function names. It would furthermore match the field name. > +{ > + return pwm->chip->can_sleep; > +} > +EXPORT_SYMBOL_GPL(pwm_cansleep); Would it make sense to check for NULL pointers here? I guess that passing NULL into the function could be considered a programming error and an oops would be okay, but in that case there's no point in making the function return an int. Also see my next comment. > + > #ifdef CONFIG_DEBUG_FS > static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) > { > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 70655a2..e2cb5c7 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -146,6 +146,8 @@ struct pwm_ops { > * @base: number of first PWM controlled by this chip > * @npwm: number of PWMs controlled by this chip > * @pwms: array of PWM devices allocated by the framework > + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep, > + * as they must while accessing PWM chips over I2C or SPI > */ > struct pwm_chip { > struct device *dev; > @@ -159,6 +161,7 @@ struct pwm_chip { > struct pwm_device * (*of_xlate)(struct pwm_chip *pc, > const struct of_phandle_args *args); > unsigned int of_pwm_n_cells; > + unsigned int can_sleep:1; What's the reason for making this a bitfield? Couldn't we just use a bool instead? Thierry
hi Thierry, On 01/26/2013 06:40 AM, Thierry Reding wrote: >> + * @pwm: PWM device >> + * >> + * It returns nonzero if accessing the PWM can sleep. >> + */ >> +int pwm_cansleep(struct pwm_device *pwm) > > I actually liked pwm_can_sleep() better. I find it to be more consistent > with the naming of other function names. It would furthermore match the > field name. I was looking at the GPIO API to suggest this name change, but you are right we should be consistent with the PWM API here. Sorry Florian. > >> +{ >> + return pwm->chip->can_sleep; >> +} >> +EXPORT_SYMBOL_GPL(pwm_cansleep); > > Would it make sense to check for NULL pointers here? I guess that > passing NULL into the function could be considered a programming error > and an oops would be okay, but in that case there's no point in making > the function return an int. Also see my next comment. While it is unlikely to happen it is better to be safe, something like this will do: return pwm ? pwm->chip->can_sleep : 0; > >> + >> #ifdef CONFIG_DEBUG_FS >> static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) >> { >> diff --git a/include/linux/pwm.h b/include/linux/pwm.h >> index 70655a2..e2cb5c7 100644 >> --- a/include/linux/pwm.h >> +++ b/include/linux/pwm.h >> @@ -146,6 +146,8 @@ struct pwm_ops { >> * @base: number of first PWM controlled by this chip >> * @npwm: number of PWMs controlled by this chip >> * @pwms: array of PWM devices allocated by the framework >> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep, >> + * as they must while accessing PWM chips over I2C or SPI >> */ >> struct pwm_chip { >> struct device *dev; >> @@ -159,6 +161,7 @@ struct pwm_chip { >> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >> const struct of_phandle_args *args); >> unsigned int of_pwm_n_cells; >> + unsigned int can_sleep:1; > > What's the reason for making this a bitfield? Couldn't we just use a > bool instead? I have also overlooked this. In my version I had the can_sleep as bool also.
Hello, Le 28/01/2013 09:45, Peter Ujfalusi a écrit : > hi Thierry, > > On 01/26/2013 06:40 AM, Thierry Reding wrote: >>> + * @pwm: PWM device >>> + * >>> + * It returns nonzero if accessing the PWM can sleep. >>> + */ >>> +int pwm_cansleep(struct pwm_device *pwm) >> >> I actually liked pwm_can_sleep() better. I find it to be more consistent >> with the naming of other function names. It would furthermore match the >> field name. > > I was looking at the GPIO API to suggest this name change, but you are right > we should be consistent with the PWM API here. > Sorry Florian. > No problem, I agree with the PWM API consistency. >> >>> +{ >>> + return pwm->chip->can_sleep; >>> +} >>> +EXPORT_SYMBOL_GPL(pwm_cansleep); >> >> Would it make sense to check for NULL pointers here? I guess that >> passing NULL into the function could be considered a programming error >> and an oops would be okay, but in that case there's no point in making >> the function return an int. Also see my next comment. > > While it is unlikely to happen it is better to be safe, something like this > will do: > > return pwm ? pwm->chip->can_sleep : 0; > Ok. And what about: BUG_ON(pwm == NULL); return pwm->chip->can_sleep; >> >>> + >>> #ifdef CONFIG_DEBUG_FS >>> static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) >>> { >>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h >>> index 70655a2..e2cb5c7 100644 >>> --- a/include/linux/pwm.h >>> +++ b/include/linux/pwm.h >>> @@ -146,6 +146,8 @@ struct pwm_ops { >>> * @base: number of first PWM controlled by this chip >>> * @npwm: number of PWMs controlled by this chip >>> * @pwms: array of PWM devices allocated by the framework >>> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep, >>> + * as they must while accessing PWM chips over I2C or SPI >>> */ >>> struct pwm_chip { >>> struct device *dev; >>> @@ -159,6 +161,7 @@ struct pwm_chip { >>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>> const struct of_phandle_args *args); >>> unsigned int of_pwm_n_cells; >>> + unsigned int can_sleep:1; >> >> What's the reason for making this a bitfield? Couldn't we just use a >> bool instead? > > I have also overlooked this. In my version I had the can_sleep as bool also. > Ok for a bool. Thank you for your reviews. I will send a v3 sometimes today. Cheers, Florian
On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote: > Hello, > > Le 28/01/2013 09:45, Peter Ujfalusi a écrit : > >hi Thierry, > > > >On 01/26/2013 06:40 AM, Thierry Reding wrote: [...] > >>>+{ > >>>+ return pwm->chip->can_sleep; > >>>+} > >>>+EXPORT_SYMBOL_GPL(pwm_cansleep); > >> > >>Would it make sense to check for NULL pointers here? I guess that > >>passing NULL into the function could be considered a programming error > >>and an oops would be okay, but in that case there's no point in making > >>the function return an int. Also see my next comment. > > > >While it is unlikely to happen it is better to be safe, something like this > >will do: > > > >return pwm ? pwm->chip->can_sleep : 0; > > > > Ok. And what about: > > BUG_ON(pwm == NULL); > return pwm->chip->can_sleep; I don't think we need that. In case pwm == NULL, dereferencing it will oops anyway. So either we make it safe and return an error code, or we let it oops without explicit BUG_ON(). Thierry
Le 28/01/2013 10:57, Thierry Reding a écrit : > On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote: >> Hello, >> >> Le 28/01/2013 09:45, Peter Ujfalusi a écrit : >>> hi Thierry, >>> >>> On 01/26/2013 06:40 AM, Thierry Reding wrote: > [...] >>>>> +{ >>>>> + return pwm->chip->can_sleep; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pwm_cansleep); >>>> >>>> Would it make sense to check for NULL pointers here? I guess that >>>> passing NULL into the function could be considered a programming error >>>> and an oops would be okay, but in that case there's no point in making >>>> the function return an int. Also see my next comment. >>> >>> While it is unlikely to happen it is better to be safe, something like this >>> will do: >>> >>> return pwm ? pwm->chip->can_sleep : 0; >>> >> >> Ok. And what about: >> >> BUG_ON(pwm == NULL); >> return pwm->chip->can_sleep; > > I don't think we need that. In case pwm == NULL, dereferencing it will > oops anyway. So either we make it safe and return an error code, or we > let it oops without explicit BUG_ON(). > Calling this function with a NULL pointer is a programming error, so there is no error codes for such errors. I propose to return bool, and let it oops if such case happens. Regards, Florian
On Mon, Jan 28, 2013 at 11:57:39AM +0100, Florian Vaussard wrote: > Le 28/01/2013 10:57, Thierry Reding a écrit : > >On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote: > >>Hello, > >> > >>Le 28/01/2013 09:45, Peter Ujfalusi a écrit : > >>>hi Thierry, > >>> > >>>On 01/26/2013 06:40 AM, Thierry Reding wrote: > >[...] > >>>>>+{ > >>>>>+ return pwm->chip->can_sleep; > >>>>>+} > >>>>>+EXPORT_SYMBOL_GPL(pwm_cansleep); > >>>> > >>>>Would it make sense to check for NULL pointers here? I guess that > >>>>passing NULL into the function could be considered a programming error > >>>>and an oops would be okay, but in that case there's no point in making > >>>>the function return an int. Also see my next comment. > >>> > >>>While it is unlikely to happen it is better to be safe, something like this > >>>will do: > >>> > >>>return pwm ? pwm->chip->can_sleep : 0; > >>> > >> > >>Ok. And what about: > >> > >>BUG_ON(pwm == NULL); > >>return pwm->chip->can_sleep; > > > >I don't think we need that. In case pwm == NULL, dereferencing it will > >oops anyway. So either we make it safe and return an error code, or we > >let it oops without explicit BUG_ON(). > > > > Calling this function with a NULL pointer is a programming error, so there > is no error codes for such errors. You could return -EINVAL if pwm == NULL. > I propose to return bool, and let it oops if such case happens. My point was that it will oops even if you don't use BUG_ON() so there isn't so much point in using it explicitly. Thierry
On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote: > Hello, > > Le 28/01/2013 09:45, Peter Ujfalusi a écrit : >> hi Thierry, >> >> On 01/26/2013 06:40 AM, Thierry Reding wrote: >>>> +{ >>>> + return pwm->chip->can_sleep; >>>> +} >>>> +EXPORT_SYMBOL_GPL(pwm_cansleep); >>> >>> Would it make sense to check for NULL pointers here? I guess that >>> passing NULL into the function could be considered a programming error >>> and an oops would be okay, but in that case there's no point in making >>> the function return an int. Also see my next comment. >> >> While it is unlikely to happen it is better to be safe, something like this >> will do: >> >> return pwm ? pwm->chip->can_sleep : 0; >> > > Ok. And what about: > > BUG_ON(pwm == NULL); > return pwm->chip->can_sleep; Let's get something straight. 1. Don't use BUG_ON() as some kind of willy nilly assert() replacement. Linus refused to have assert() in the kernel because assert() gets not only over-used, but also gets inappropriately used too. _Only_ _ever_ use BUG_ON() if continuing is going to cause user noticable data loss which is not reportable to userspace. In other words, block device queue corruption or the like - where bringing the system down is going to _save_ the system from itself. Otherwise, return an error and/or use WARN_ON(). 2. If you want a slow kernel, then by all means check your arguments to your functions. While you're at it, why not check that strings which are passed contain only the characters you expect them to? And, if you're bothering to check against a NULL pointer, what about NULL+1 pointers which are also invalid? Why not invent some function to ensure that the pointer is a valid kernel pointer. Maybe you'll have to interate the vmalloc lists too - yay, more code to be executed! That must be good! In your example, if you're going to check that pwm is non-NULL, what if pwm->chip is non-NULL? How far do you take this? Or... just like most of the core kernel does, it does _not_ verify on function entry that the pointer is "correct" unless it is explicitly defined that the function may take a NULL pointer (like kfree()). Everything else just goes right on and does the dereference - and if the pointer was wrong, we hope that the MMU faults and we get a kernel oops. Have a read through the code in fs/ or kernel/ and see how many functions you can spot in there which validate their pointers which aren't dealing with data from userland. You'll find almost no function checking that an inode pointer is not NULL. Or a struct file pointer. Or a struct path pointer... etc. Yet, you come to ARM code, and it seems "popular" that pointer arguments need to be verified on every single function call. Why is this? I don't know if Andrew would like to inject something here (I've added him) on this subject...
Hello, Le 28/01/2013 16:01, Russell King - ARM Linux a écrit : > On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote: >> Hello, >> >> Le 28/01/2013 09:45, Peter Ujfalusi a écrit : >>> hi Thierry, >>> >>> On 01/26/2013 06:40 AM, Thierry Reding wrote: >>>>> +{ >>>>> + return pwm->chip->can_sleep; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pwm_cansleep); >>>> >>>> Would it make sense to check for NULL pointers here? I guess that >>>> passing NULL into the function could be considered a programming error >>>> and an oops would be okay, but in that case there's no point in making >>>> the function return an int. Also see my next comment. >>> >>> While it is unlikely to happen it is better to be safe, something like this >>> will do: >>> >>> return pwm ? pwm->chip->can_sleep : 0; >>> >> >> Ok. And what about: >> >> BUG_ON(pwm == NULL); >> return pwm->chip->can_sleep; > > Let's get something straight. > > 1. Don't use BUG_ON() as some kind of willy nilly assert() replacement. > Linus refused to have assert() in the kernel because assert() gets not > only over-used, but also gets inappropriately used too. > > _Only_ _ever_ use BUG_ON() if continuing is going to cause user > noticable data loss which is not reportable to userspace. In other > words, block device queue corruption or the like - where bringing the > system down is going to _save_ the system from itself. > > Otherwise, return an error and/or use WARN_ON(). > > 2. If you want a slow kernel, then by all means check your arguments to > your functions. While you're at it, why not check that strings which > are passed contain only the characters you expect them to? And, if > you're bothering to check against a NULL pointer, what about NULL+1 > pointers which are also invalid? Why not invent some function to > ensure that the pointer is a valid kernel pointer. Maybe you'll have > to interate the vmalloc lists too - yay, more code to be executed! > That must be good! > > In your example, if you're going to check that pwm is non-NULL, what > if pwm->chip is non-NULL? How far do you take this? > > Or... just like most of the core kernel does, it does _not_ verify on > function entry that the pointer is "correct" unless it is explicitly > defined that the function may take a NULL pointer (like kfree()). > Everything else just goes right on and does the dereference - and if > the pointer was wrong, we hope that the MMU faults and we get a kernel > oops. > > Have a read through the code in fs/ or kernel/ and see how many functions > you can spot in there which validate their pointers which aren't dealing > with data from userland. > > You'll find almost no function checking that an inode pointer is not NULL. > Or a struct file pointer. Or a struct path pointer... etc. > > Yet, you come to ARM code, and it seems "popular" that pointer arguments > need to be verified on every single function call. Why is this? > > I don't know if Andrew would like to inject something here (I've added > him) on this subject... > The v3 does not contain the check. Thank you, Florian
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 4a13da4..e737f5f 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm) } EXPORT_SYMBOL_GPL(devm_pwm_put); +/** + * pwm_cansleep() - report whether pwm access will sleep + * @pwm: PWM device + * + * It returns nonzero if accessing the PWM can sleep. + */ +int pwm_cansleep(struct pwm_device *pwm) +{ + return pwm->chip->can_sleep; +} +EXPORT_SYMBOL_GPL(pwm_cansleep); + #ifdef CONFIG_DEBUG_FS static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) { diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 70655a2..e2cb5c7 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -146,6 +146,8 @@ struct pwm_ops { * @base: number of first PWM controlled by this chip * @npwm: number of PWMs controlled by this chip * @pwms: array of PWM devices allocated by the framework + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep, + * as they must while accessing PWM chips over I2C or SPI */ struct pwm_chip { struct device *dev; @@ -159,6 +161,7 @@ struct pwm_chip { struct pwm_device * (*of_xlate)(struct pwm_chip *pc, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; + unsigned int can_sleep:1; }; #if IS_ENABLED(CONFIG_PWM) @@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id); struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np, const char *con_id); void devm_pwm_put(struct device *dev, struct pwm_device *pwm); + +int pwm_cansleep(struct pwm_device *pwm); #else static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data) { @@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev, static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm) { } + +static inline int pwm_cansleep(struct pwm_device *pwm) +{ + return 0; +} #endif struct pwm_lookup {
Calls to some external PWM chips can sleep. To help users, add pwm_cansleep() API. Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> --- drivers/pwm/core.c | 12 ++++++++++++ include/linux/pwm.h | 10 ++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)