Re: [PATCH v4 23/79] staging: media: ipu3: use pm_runtime_resume_and_get()

[Johan Hovold]

On Fri, Apr 30, 2021 at 06:03:38PM +0100, Jonathan Cameron wrote:
> On Wed, 28 Apr 2021 16:51:44 +0200
> Mauro Carvalho Chehab  wrote:
> 
> > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal 
> > with usage counter")
> > added pm_runtime_resume_and_get() in order to automatically handle
> > dev->power.usage_count decrement on errors.
> > 
> > Use the new API, in order to cleanup the error check logic.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> Could add that pm_runtime_put() should have been pm_runtime_put_noidle()
> inorder to not potentially result in a call to runtime suspend when
> we never resumed in the first place.

No, that would never happen anyway and any pm_runtime_put() will do
even if pm_runtime_put_noidle() is the natural choice in this case to
balance the counter.

> Otherwise reasonable cleanup.
> 
> Reviewed-by: Jonathan Cameron 
> 
> > ---
> >  drivers/staging/media/ipu3/ipu3.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/ipu3.c 
> > b/drivers/staging/media/ipu3/ipu3.c
> > index ee1bba6bdcac..8e1e9e46e604 100644
> > --- a/drivers/staging/media/ipu3/ipu3.c
> > +++ b/drivers/staging/media/ipu3/ipu3.c
> > @@ -392,10 +392,9 @@ int imgu_s_stream(struct imgu_device *imgu, int enable)
> > }
> >  
> > /* Set Power */
> > -   r = pm_runtime_get_sync(dev);
> > +   r = pm_runtime_resume_and_get(dev);
> > if (r < 0) {
> > dev_err(dev, "failed to set imgu power\n");
> > -   pm_runtime_put(dev);
> > return r;
> > }

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem

[Johan Hovold]

On Thu, Apr 29, 2021 at 12:18:16PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 17:50:08 +0200
> Johan Hovold  escreveu:
> 
> > On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> 
> > > 1. despite its name, this is actually a PM runtime resume call,
> > >but some developers didn't seem to realize that, as I got this
> > >pattern on some drivers:
> > > 
> > > pm_runtime_get_sync(>dev);
> > > pm_runtime_disable(>dev);
> > > pm_runtime_set_suspended(>dev);
> > > pm_runtime_put_noidle(>dev);
> > > 
> > >It makes no sense to resume PM just to suspend it again ;-)  
> > 
> > This is perfectly alright. Take a look at ov7740_remove() for example:
> > 
> > pm_runtime_get_sync(>dev);
> > pm_runtime_disable(>dev);
> > pm_runtime_set_suspended(>dev);
> > pm_runtime_put_noidle(>dev);
> > 
> > ov7740_set_power(ov7740, 0);
> > 
> > There's an explicit power-off after balancing the PM count and this will
> > work regardless of the power state when entering this function.
> 
> Ok, but this should equally work:
> 
>   pm_runtime_disable(>dev);
>   pm_runtime_set_suspended(>dev);
>   
>   ov7740_set_power(ov7740, 0);
> 
> as there's no additional cleanup made on this particular driver
> between pm_runtime_get_sync() and pm_runtime_put_noidle().

No, that would break the driver as I pointed out to you yesterday:

https://lore.kernel.org/r/yimg1klspkfsa...@hovoldconsulting.com

If the device is already suspended when remove is called then you'll
end up with an unbalanced call to ov7740_set_power() that will try to
disable an already disabled clock.

> > So this has nothing to do with pm_runtime_get_sync() per se.
> 
> Yes, but some patches on this series are cleaning up the driver release
> logic.

You mentioned this example as an argument against using
pm_runtime_get_sync(), which I don't think makes sense.

> > > 2. Usual *_get() methods only increment their use count on success,
> > >but pm_runtime_get_sync() increments it unconditionally. Due to
> > >that, several drivers were mistakenly not calling
> > >pm_runtime_put_noidle() when it fails;  
> > 
> > Sure, but pm_runtime_get_async() also works this way. You just won't be
> > notified if the async resume fails.
> 
> Granted, it makes sense along the pm_runtime kAPI.
> 
> It is inconsistent with the behavior of kobject_get*() and other
> *_get*() methods that are based or inspired on it, as, on those, the
> operations are atomic: either everything succeeds and it doesn't return
> an error, or the usage counter is not incremented and the object
> state doesn't change after the call.

Right, and I'm aware that some people have overlooked this. But its not
the end of the world since hardly any driver can handle resume failures
properly anyway. 

This is mostly just an exercise to shut up static checkers.

> > > 3. The name of the new variant is a lot clearer:
> > >   pm_runtime_resume_and_get()
> > > As its same clearly says that this is a PM runtime resume function,
> > > that also increments the usage counter on success;  
> > 
> > It also introduced an inconsistency in the API and does not pair as well
> > with the pm_runtime_put variants.
> 
> Agreed. A name that would be more consistent with PM runtime would
> probably be:
> 
>   pm_runtime_resume_if_get()

Naw, since the get part always succeeds.

It should start with pm_runtime_get, but pm_runtime_get_sync() is
unfortunately taken.

> as there are already:
> 
>   pm_runtime_get_if_in_use()
>   pm_runtime_get_if_active()
> 
> But any such discussions are out of the scope of this patchset ;-)

Right.

> > > 4. Consistency: we did similar changes subsystem wide with
> > >for instance strlcpy() and strcpy() that got replaced by
> > >strscpy(). Having all drivers using the same known-to-be-safe
> > >methods is a good thing;  
> > 
> > It's not known to be safe; there are ways to get also this interface
> > wrong as for example this series has shown.
> 
> Very true. Yet, it is a lot simpler to use functions that won't change
> the state of the objects when returning an error, as this is by far
> the most common pattern within the Kernel.

A resume failure does change the state (and needs to be recovered from),
but I get what you're saying.

> Human brains are trained to identify certain patterns. When there's
> something using a similar pattern, but with a different be

Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem

[Johan Hovold]

On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> During the review of the patches from unm.edu, one of the patterns
> I noticed is the amount of patches trying to fix pm_runtime_get_sync()
> calls.
> 
> After analyzing the feedback from version 1 of this series, I noticed
> a few other weird behaviors at the PM runtime resume code. So, this
> series start addressing some bugs and issues at the current code.
> Then, it gets rid of pm_runtime_get_sync() at the media subsystem
> (with 2 exceptions).
> 
> It should be noticed that
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with 
> usage counter")
> added a new method to does a pm_runtime get, which increments
> the usage count only on success.
> 
> The rationale of getting rid of pm_runtime_get_sync() is:
> 
> 1. despite its name, this is actually a PM runtime resume call,
>but some developers didn't seem to realize that, as I got this
>pattern on some drivers:
> 
> pm_runtime_get_sync(>dev);
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>dev);
> 
>It makes no sense to resume PM just to suspend it again ;-)

This is perfectly alright. Take a look at ov7740_remove() for example:

pm_runtime_get_sync(>dev);
pm_runtime_disable(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_put_noidle(>dev);

ov7740_set_power(ov7740, 0);

There's an explicit power-off after balancing the PM count and this will
work regardless of the power state when entering this function.

So this has nothing to do with pm_runtime_get_sync() per se.

> 2. Usual *_get() methods only increment their use count on success,
>but pm_runtime_get_sync() increments it unconditionally. Due to
>that, several drivers were mistakenly not calling
>pm_runtime_put_noidle() when it fails;

Sure, but pm_runtime_get_async() also works this way. You just won't be
notified if the async resume fails.

> 3. The name of the new variant is a lot clearer:
>   pm_runtime_resume_and_get()
> As its same clearly says that this is a PM runtime resume function,
> that also increments the usage counter on success;

It also introduced an inconsistency in the API and does not pair as well
with the pm_runtime_put variants.

> 4. Consistency: we did similar changes subsystem wide with
>for instance strlcpy() and strcpy() that got replaced by
>strscpy(). Having all drivers using the same known-to-be-safe
>methods is a good thing;

It's not known to be safe; there are ways to get also this interface
wrong as for example this series has shown.

> 5. Prevent newer drivers to copy-and-paste a code that it would
>be easier to break if they don't truly understand what's behind
>the scenes.

Cargo-cult programming always runs that risk.

> This series replace places  pm_runtime_get_sync(), by calling
> pm_runtime_resume_and_get() instead.
> 
> This should help to avoid future mistakes like that, as people
> tend to use the existing drivers as examples for newer ones.

The only valid point about and use for pm_runtime_resume_and_get() is to
avoid leaking a PM usage count reference in the unlikely case that
resume fails (something which hardly any driver implements recovery
from anyway).

It's a convenience wrapper that saves you from writing one extra line in
some cases (depending on how you implement runtime-pm support) and not a
silver bullet against bugs.
 
> compile-tested only.
> 
> Patches 1 to 7 fix some issues that already exists at the current
> PM runtime code;
> 
> patches 8 to 20 fix some usage_count problems that still exists
> at the media subsystem;
> 
> patches 21 to 78 repaces pm_runtime_get_sync() by 
> pm_runtime_resume_and_get();
> 
> Patch 79 (and a hunk on patch 78) documents the two exceptions
> where pm_runtime_get_sync() will still be used for now.
> 
> ---
> 
> v4:
> - Added a couple of additional fixes at existing PM runtime code;
> - Some patches are now more conservative in order to avoid causing
>  regressions.
> v3:
> - fix a compilation error;
> v2:
> - addressed pointed issues and fixed a few other PM issues.

This really doesn't say much more than "changed stuff" so kinda hard to
track if review feedback has been taken into account for example.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 20/79] staging: media: rkvdec: fix pm_runtime_get_sync() usage count

[Johan Hovold]

On Wed, Apr 28, 2021 at 04:51:41PM +0200, Mauro Carvalho Chehab wrote:
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with 
> usage counter")
> in order to properly decrement the usage counter and avoid memory
> leaks.

Again, there is no memory leak here either. Just a potential PM usage
counter leak.

> Reviewed-by: Ezequiel Garcia 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c 
> b/drivers/staging/media/rkvdec/rkvdec.c
> index d821661d30f3..8c17615f3a7a 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -658,7 +658,7 @@ static void rkvdec_device_run(void *priv)
>   if (WARN_ON(!desc))
>   return;
>  
> - ret = pm_runtime_get_sync(rkvdec->dev);
> + ret = pm_runtime_resume_and_get(rkvdec->dev);
>   if (ret < 0) {
>   rkvdec_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
>   return;

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/78] staging: media: vde: use pm_runtime_resume_and_get()

[Johan Hovold]

On Tue, Apr 27, 2021 at 11:22:50AM +0200, Mauro Carvalho Chehab wrote:
> Hi Dmitry,
> 
> Em Sat, 24 Apr 2021 10:35:22 +0300
> Dmitry Osipenko  escreveu:
> 
> > 24.04.2021 09:44, Mauro Carvalho Chehab пишет:
> > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal 
> > > with usage counter")
> > > added pm_runtime_resume_and_get() in order to automatically handle
> > > dev->power.usage_count decrement on errors.
> > > 
> > > Use the new API, in order to cleanup the error check logic.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab 
> > > ---
> > >  drivers/staging/media/tegra-vde/vde.c | 16 ++--
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/tegra-vde/vde.c 
> > > b/drivers/staging/media/tegra-vde/vde.c
> > > index 28845b5bafaf..8936f140a246 100644
> > > --- a/drivers/staging/media/tegra-vde/vde.c
> > > +++ b/drivers/staging/media/tegra-vde/vde.c
> > > @@ -775,9 +775,9 @@ static int tegra_vde_ioctl_decode_h264(struct 
> > > tegra_vde *vde,
> > >   if (ret)
> > >   goto release_dpb_frames;
> > >  
> > > - ret = pm_runtime_get_sync(dev);
> > > + ret = pm_runtime_resume_and_get(dev);
> > >   if (ret < 0)
> > > - goto put_runtime_pm;
> > > + goto unlock;
> > >  
> > >   /*
> > >* We rely on the VDE registers reset value, otherwise VDE
> > > @@ -843,6 +843,8 @@ static int tegra_vde_ioctl_decode_h264(struct 
> > > tegra_vde *vde,
> > >  put_runtime_pm:
> > >   pm_runtime_mark_last_busy(dev);
> > >   pm_runtime_put_autosuspend(dev);
> > > +
> > > +unlock:
> > >   mutex_unlock(>lock);
> > >  
> > >  release_dpb_frames:
> > > @@ -1069,8 +1071,8 @@ static int tegra_vde_probe(struct platform_device 
> > > *pdev)
> > >* power-cycle it in order to put hardware into a predictable lower
> > >* power state.
> > >*/
> > > - pm_runtime_get_sync(dev);
> > > - pm_runtime_put(dev);
> > > + if (pm_runtime_resume_and_get(dev) >= 0)
> > > + pm_runtime_put(dev);
> > >  
> > >   return 0;
> > >  
> > > @@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device 
> > > *pdev)
> > >  {
> > >   struct tegra_vde *vde = platform_get_drvdata(pdev);
> > >   struct device *dev = >dev;
> > > + int ret;
> > >  
> > > - pm_runtime_get_sync(dev);
> > > + ret = pm_runtime_resume_and_get(dev);
> > >   pm_runtime_dont_use_autosuspend(dev);
> > >   pm_runtime_disable(dev);
> > >  
> > > @@ -1097,7 +1100,8 @@ static int tegra_vde_remove(struct platform_device 
> > > *pdev)
> > >* Balance RPM state, the VDE power domain is left ON and hardware
> > >* is clock-gated. It's safe to reboot machine now.
> > >*/
> > > - pm_runtime_put_noidle(dev);
> > > + if (ret >= 0)
> > > + pm_runtime_put_noidle(dev);
> > >   clk_disable_unprepare(vde->clk);
> > >  
> > >   misc_deregister(>miscdev);
> > >   
> > 
> > Hello Mauro,
> > 
> > Thank you very much for the patch. It looks to me that the original
> > variant was a bit simpler, this patch adds more code lines without
> > changing the previous behaviour. Or am I missing something?

I agree, the above does not look like an improvement at all.

> While on several places the newer code is simpler, the end goal here is
> to replace all occurrences of pm_runtime_get_sync() from the media 
> subsystem, due to the number of problems we're having with this:
> 
> 1. despite its name, this is actually a PM runtime resume call,
>but some developers didn't seem to realize that, as I got this
>pattern on some drivers:
> 
> pm_runtime_get_sync(>dev);
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>dev);
> 
>It makes no sense to resume PM just to suspend it again ;-)

It very well may. You're resuming the device and leaving it a defined
power state before balancing the PM count, cleaning up and unbinding the
driver.

>The name of the new variant is a lot clearer:
>   pm_runtime_resume_and_get()

For people not used to the API perhaps.

> 2. Usual *_get() methods only increment their use count on success,
>but pm_runtime_get_sync() increments it unconditionally. Due to
>that, several drivers were mistakenly not calling
>pm_runtime_put_noidle() when it fails;

As I mentioned elsewhere, all pm_runtime_get calls increment the usage
count so the API is consistent.
 
> 3. Consistency: we did similar changes subsystem wide with
>for instance strlcpy() and strcpy() that got replaced by
>strscpy(). Having all drivers using the same known-to-be-safe
>methods is a good thing;

There's no know-to-be safe API. People will find ways to get this wrong
too.

And the old interface isn't going away from the kernel even if you
manage to not use it in media.

> 4. Prevent newer drivers to copy-and-paste a code that it would
>be easier to break if they don't truly understand what's behind
>the scenes.

Johan
___

Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c

[Johan Hovold]

On Fri, Feb 12, 2021 at 03:25:29PM +0530, Viresh Kumar wrote:
> On 12-02-21, 10:52, Johan Hovold wrote:
> > But what will the checkpatch crew then work on?
> 
> Lol..
> 
> > Better staging than the rest of the kernel.
> 
> [ /me wondering on who stops them from sending patches for rest of the
> kernel ? ]

Ideally maintainers should at least push back when they do.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c

[Johan Hovold]

On Fri, Feb 12, 2021 at 02:51:30PM +0530, Viresh Kumar wrote:
> On 12-02-21, 10:17, Greg KH wrote:
> > On Fri, Feb 12, 2021 at 02:39:26PM +0530, Viresh Kumar wrote:
> > > On 12-02-21, 13:48, Pritthijit Nath wrote:
> > > > This change fixes a checkpatch CHECK style issue for "Alignment should 
> > > > match
> > > > open parenthesis".
> > > > 
> > > > Signed-off-by: Pritthijit Nath 
> > > > ---
> > > >  drivers/staging/greybus/hid.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/greybus/hid.c 
> > > > b/drivers/staging/greybus/hid.c
> > > > index ed706f39e87a..a56c3fb5d35a 100644
> > > > --- a/drivers/staging/greybus/hid.c
> > > > +++ b/drivers/staging/greybus/hid.c
> > > > @@ -221,8 +221,8 @@ static void gb_hid_init_reports(struct gb_hid *ghid)
> > > >  }
> > > > 
> > > >  static int __gb_hid_get_raw_report(struct hid_device *hid,
> > > > -   unsigned char report_number, __u8 *buf, size_t count,
> > > > -   unsigned char report_type)
> > > > +  unsigned char report_number, __u8 
> > > > *buf, size_t count,
> > > > +  unsigned char report_type)
> > > >  {
> > > > struct gb_hid *ghid = hid->driver_data;
> > > > int ret;
> > > 
> > > I can't even count the number of attempts we have seen in previous
> > > years to make checkpatch --strict happy for greybus.
> > > 
> > > I say we make this change once and for all across greybus, so we don't
> > > have to review or NAK someone afterwards.
> > > 
> > > Should I send a patch for this ?
> > 
> > Sure, but note that over time, checkpatch adds new things so there will
> > always be something to change in here, until we move it out of the
> > drivers/staging/ area :)
> 
> Right, though I wasn't worried about other checkpatch warning but
> specially the "alignment - parenthesis" one. Everyone (specially
> newbies) want to fix that everywhere :)

But what will the checkpatch crew then work on? Better staging than the
rest of the kernel.

Perhaps just let them keep at it until we move the rest out (the price
we pay for taking this through staging apparently).

But there can't be that many instances left of this alignment
"violation" in staging/greybus (~4 it seems) if you want to get it over
with.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: greybus: Fixed a misspelling in hid.c

[Johan Hovold]

On Fri, Feb 12, 2021 at 01:48:35PM +0530, Pritthijit Nath wrote:
> Fixed the spelling of 'transfered' to 'transferred'.
> 
> Signed-off-by: Pritthijit Nath 
> ---
>  drivers/staging/greybus/hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c
> index a56c3fb5d35a..6b19ff4743a9 100644
> --- a/drivers/staging/greybus/hid.c
> +++ b/drivers/staging/greybus/hid.c
> @@ -254,7 +254,7 @@ static int __gb_hid_output_raw_report(struct hid_device 
> *hid, __u8 *buf,
> 
>   ret = gb_hid_set_report(ghid, report_type, report_id, buf, len);
>   if (report_id && ret >= 0)
> - ret++; /* add report_id to the number of transfered bytes */
> + ret++; /* add report_id to the number of transferrid bytes */

You now misspelled transferred in a different way.

> 
>   return 0;
>  }

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: greybus: vibrator: use proper API for vibrator devices

[Johan Hovold]

On Wed, Jan 06, 2021 at 01:04:04PM +0100, Johan Hovold wrote:
> On Tue, Jan 05, 2021 at 04:19:02PM +0100, Greg Kroah-Hartman wrote:
> > The correct user/kernel api for vibrator devices is the Input rumble
> > api, not a random sysfs file like the greybus vibrator driver currently
> > uses.
> > 
> > Add support for the correct input api to the vibrator driver so that it
> > hooks up to the kernel and userspace correctly.
> > 
> > Cc: Johan Hovold 
> > Cc: Alex Elder 
> > Signed-off-by: Greg Kroah-Hartman 
> > ---
> >  drivers/staging/greybus/vibrator.c | 59 ++
> >  1 file changed, 59 insertions(+)
> > 
> > diff --git a/drivers/staging/greybus/vibrator.c 
> > b/drivers/staging/greybus/vibrator.c
> > index 0e2b188e5ca3..94110cadb5bd 100644
> > --- a/drivers/staging/greybus/vibrator.c
> > +++ b/drivers/staging/greybus/vibrator.c
> > @@ -13,13 +13,18 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  struct gb_vibrator_device {
> > struct gb_connection*connection;
> > +   struct input_dev*input;
> > struct device   *dev;
> > int minor;  /* vibrator minor number */
> > struct delayed_work delayed_work;
> > +   boolrunning;
> > +   boolon;
> > +   struct work_struct  play_work;
> >  };
> >  
> >  /* Greybus Vibrator operation types */
> > @@ -36,6 +41,7 @@ static int turn_off(struct gb_vibrator_device *vib)
> >  
> > gb_pm_runtime_put_autosuspend(bundle);
> >  
> > +   vib->on = false;
> 
> You update but never seem to actually use vib->on.
> 
> > return ret;
> >  }
> >  
> > @@ -59,11 +65,48 @@ static int turn_on(struct gb_vibrator_device *vib, u16 
> > timeout_ms)
> > return ret;
> > }
> >  
> > +   vib->on = true;
> > schedule_delayed_work(>delayed_work, msecs_to_jiffies(timeout_ms));
> >  
> > return 0;
> >  }
> >  
> > +static void gb_vibrator_play_work(struct work_struct *work)
> > +{
> > +   struct gb_vibrator_device *vib =
> > +   container_of(work, struct gb_vibrator_device, play_work);
> > +
> > +   if (vib->running)
> 
> Is this test inverted?
> 
> > +   turn_off(vib);
> > +   else
> > +   turn_on(vib, 100);
> 
> Why 100 ms?
> 
> Shouldn't it just be left on indefinitely with the new API?
> 
> > +}
> > +
> > +static int gb_vibrator_play_effect(struct input_dev *input, void *data,
> > +  struct ff_effect *effect)
> > +{
> > +   struct gb_vibrator_device *vib = input_get_drvdata(input);
> > +   int level;
> > +
> > +   level = effect->u.rumble.strong_magnitude;
> > +   if (!level)
> > +   level = effect->u.rumble.weak_magnitude;
> > +
> > +   vib->running = level;
> > +   schedule_work(>play_work);
> > +   return 0;
> > +}
> > +
> > +static void gb_vibrator_close(struct input_dev *input)
> > +{
> > +   struct gb_vibrator_device *vib = input_get_drvdata(input);
> > +
> > +   cancel_delayed_work_sync(>delayed_work);
> > +   cancel_work_sync(>play_work);
> > +   turn_off(vib);
> > +   vib->running = false;
> > +}
> > +
> >  static void gb_vibrator_worker(struct work_struct *work)
> >  {
> > struct delayed_work *delayed_work = to_delayed_work(work);
> > @@ -169,10 +212,26 @@ static int gb_vibrator_probe(struct gb_bundle *bundle,
> >  
> > INIT_DELAYED_WORK(>delayed_work, gb_vibrator_worker);
> >  
> > +   INIT_WORK(>play_work, gb_vibrator_play_work);
> 
> Hmm. Looks like you forgot to actually allocate the input device...
> 
> > +   vib->input->name = "greybus-vibrator";
> > +   vib->input->close = gb_vibrator_close;
> > +   vib->input->dev.parent = >dev;
> > +   vib->input->id.bustype = BUS_HOST;
> > +
> > +   input_set_drvdata(vib->input, vib);
> > +   input_set_capability(vib->input, EV_FF, FF_RUMBLE);
> > +
> > +   retval = input_ff_create_memless(vib->input, NULL,
> > +gb_vibrator_play_effect);
> > +   if (retval)
> > +   goto err_device_remove;
> > +

Oh, and you forgot to register the input device here too (and deregister
in remove()).

> > gb_pm_runtime_put_autosuspend(bundle);
> >  
> > return 0;
> >  
> > +err_device_remove:
> > +   device_unregister(vib->dev);
> 
> I know you're removing this in the next patch, but as the class device
> has been registered you need to cancel the delayed work and turn off the
> motor here too (side note: looks like this is done in the wrong order in
> remove()).
> 
> >  err_ida_remove:
> > ida_simple_remove(, vib->minor);
> >  err_connection_disable:
 
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: greybus: vibrator: use proper API for vibrator devices

[Johan Hovold]

On Tue, Jan 05, 2021 at 04:19:02PM +0100, Greg Kroah-Hartman wrote:
> The correct user/kernel api for vibrator devices is the Input rumble
> api, not a random sysfs file like the greybus vibrator driver currently
> uses.
> 
> Add support for the correct input api to the vibrator driver so that it
> hooks up to the kernel and userspace correctly.
> 
> Cc: Johan Hovold 
> Cc: Alex Elder 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/staging/greybus/vibrator.c | 59 ++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/staging/greybus/vibrator.c 
> b/drivers/staging/greybus/vibrator.c
> index 0e2b188e5ca3..94110cadb5bd 100644
> --- a/drivers/staging/greybus/vibrator.c
> +++ b/drivers/staging/greybus/vibrator.c
> @@ -13,13 +13,18 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  struct gb_vibrator_device {
>   struct gb_connection*connection;
> + struct input_dev*input;
>   struct device   *dev;
>   int minor;  /* vibrator minor number */
>   struct delayed_work delayed_work;
> + boolrunning;
> + boolon;
> + struct work_struct  play_work;
>  };
>  
>  /* Greybus Vibrator operation types */
> @@ -36,6 +41,7 @@ static int turn_off(struct gb_vibrator_device *vib)
>  
>   gb_pm_runtime_put_autosuspend(bundle);
>  
> + vib->on = false;

You update but never seem to actually use vib->on.

>   return ret;
>  }
>  
> @@ -59,11 +65,48 @@ static int turn_on(struct gb_vibrator_device *vib, u16 
> timeout_ms)
>   return ret;
>   }
>  
> + vib->on = true;
>   schedule_delayed_work(>delayed_work, msecs_to_jiffies(timeout_ms));
>  
>   return 0;
>  }
>  
> +static void gb_vibrator_play_work(struct work_struct *work)
> +{
> + struct gb_vibrator_device *vib =
> + container_of(work, struct gb_vibrator_device, play_work);
> +
> + if (vib->running)

Is this test inverted?

> + turn_off(vib);
> + else
> + turn_on(vib, 100);

Why 100 ms?

Shouldn't it just be left on indefinitely with the new API?

> +}
> +
> +static int gb_vibrator_play_effect(struct input_dev *input, void *data,
> +struct ff_effect *effect)
> +{
> + struct gb_vibrator_device *vib = input_get_drvdata(input);
> + int level;
> +
> + level = effect->u.rumble.strong_magnitude;
> + if (!level)
> + level = effect->u.rumble.weak_magnitude;
> +
> + vib->running = level;
> + schedule_work(>play_work);
> + return 0;
> +}
> +
> +static void gb_vibrator_close(struct input_dev *input)
> +{
> + struct gb_vibrator_device *vib = input_get_drvdata(input);
> +
> + cancel_delayed_work_sync(>delayed_work);
> + cancel_work_sync(>play_work);
> + turn_off(vib);
> + vib->running = false;
> +}
> +
>  static void gb_vibrator_worker(struct work_struct *work)
>  {
>   struct delayed_work *delayed_work = to_delayed_work(work);
> @@ -169,10 +212,26 @@ static int gb_vibrator_probe(struct gb_bundle *bundle,
>  
>   INIT_DELAYED_WORK(>delayed_work, gb_vibrator_worker);
>  
> + INIT_WORK(>play_work, gb_vibrator_play_work);

Hmm. Looks like you forgot to actually allocate the input device...

> + vib->input->name = "greybus-vibrator";
> + vib->input->close = gb_vibrator_close;
> + vib->input->dev.parent = >dev;
> + vib->input->id.bustype = BUS_HOST;
> +
> + input_set_drvdata(vib->input, vib);
> + input_set_capability(vib->input, EV_FF, FF_RUMBLE);
> +
> + retval = input_ff_create_memless(vib->input, NULL,
> +  gb_vibrator_play_effect);
> + if (retval)
> + goto err_device_remove;
> +
>   gb_pm_runtime_put_autosuspend(bundle);
>  
>   return 0;
>  
> +err_device_remove:
> + device_unregister(vib->dev);

I know you're removing this in the next patch, but as the class device
has been registered you need to cancel the delayed work and turn off the
motor here too (side note: looks like this is done in the wrong order in
remove()).

>  err_ida_remove:
>   ida_simple_remove(, vib->minor);
>  err_connection_disable:

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 09/24] wfx: add hwio.c/hwio.h

[Johan Hovold]

On Mon, Jan 04, 2021 at 03:34:10PM +0300, Dan Carpenter wrote:

> There is a Smatch warning for this, but I hadn't looked at the results
> in a while. :/  I'm not sure how many are valid.  Some kind of
> annotation would be nice.

> drivers/usb/class/usblp.c:593 usblp_ioctl() error: doing dma on the stack 
> ()
> drivers/usb/serial/iuu_phoenix.c:542 iuu_uart_flush() error: doing dma on the 
> stack ()

I only looked at these two but they are are indeed valid, and I've now
fixed them up.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH -next] greybus/audio_helper: Add missing unlock to avoid mismatched lock

[Johan Hovold]

On Mon, Dec 21, 2020 at 09:02:46PM +0800, Zheng Yongjun wrote:
> Fix a missing unlock in the error branch.
> 
> Signed-off-by: Zheng Yongjun 
> ---
>  drivers/staging/greybus/audio_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/greybus/audio_helper.c 
> b/drivers/staging/greybus/audio_helper.c
> index 237531ba60f3..293675dbea10 100644
> --- a/drivers/staging/greybus/audio_helper.c
> +++ b/drivers/staging/greybus/audio_helper.c
> @@ -135,6 +135,7 @@ int gbaudio_dapm_free_controls(struct 
> snd_soc_dapm_context *dapm,
>   if (!w) {
>   dev_err(dapm->dev, "%s: widget not found\n",
>   widget->name);
> + mutex_unlock(>card->dapm_mutex);
>   return -EINVAL;
>   }
>   widget++;

This has already been fixed in mainline by your colleague:

e77b259f67ab ("staging: greybus: audio: Fix possible leak free widgets 
in gbaudio_dapm_free_controls")

It seems you're all working on reports from your "Hulk Robot" so perhaps
you can try to coordinate that internally.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: audio: Fix possible leak free widgets in gbaudio_dapm_free_controls

[Johan Hovold]

On Fri, Dec 11, 2020 at 08:29:22PM +0800, wanghai (M) wrote:
> 
> 在 2020/12/8 17:35, Johan Hovold 写道:
> > On Sat, Dec 05, 2020 at 06:38:27PM +0800, Wang Hai wrote:
> >> In gbaudio_dapm_free_controls(), if one of the widgets is not found, an 
> >> error
> >> will be returned directly, which will cause the rest to be unable to be 
> >> freed,
> >> resulting in leak.
> >>
> >> This patch fixes the bug. If if one of them is not found, just skip and 
> >> free the others.
> > Apart from the typo, please break your lines at 72 columns or so (not
> > needed for the Fixes tag).
>
> Thanks for review,  Do I need to send a v2 patch to change the commit msg?

I'm not sure your mail reached the lists since it contains HTML, but to
answer your question: Please do resend. If you can make the maintainers'
life any easier that's always a good idea.

You should include the Reviewed-by tags you've gotten so far when
resending as long as you only update the commit message.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Add TODO item about modernizing the pwm code

[Johan Hovold]

On Tue, Dec 08, 2020 at 11:16:07AM +0100, Uwe Kleine-König wrote:
> drivers/staging/greybus/pwm.c uses the old style PWM callbacks, new drivers
> should stick to the atomic API instead.
> 
> Signed-off-by: Uwe Kleine-König 
> ---
> On 12/8/20 10:39 AM, Johan Hovold wrote:
> > No sign off?
> > 
> > Please also add a staging prefix since this part of greybus still lives
> > there.
> 
> That after all these years I still fail occasionally to add a sign-off
> ... /me shakes his head about himself.

**it happens. :)

> Anyhow, here comes a v2, also with the requested prefix.

Thanks.

Acked-by: Johan Hovold 

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] greybus: Add TODO item about modernizing the pwm code

[Johan Hovold]

On Fri, Dec 04, 2020 at 07:33:35PM +0100, Uwe Kleine-König wrote:
> drivers/staging/greybus/pwm.c uses the old style PWM callbacks, new drivers
> should stick to the atomic API instead.
> ---

No sign off?

Please also add a staging prefix since this part of greybus still lives
there.

>  drivers/staging/greybus/TODO | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/greybus/TODO b/drivers/staging/greybus/TODO
> index 31f1f2cb401c..6461e0132fe3 100644
> --- a/drivers/staging/greybus/TODO
> +++ b/drivers/staging/greybus/TODO
> @@ -1,3 +1,5 @@
>  * Convert all uses of the old GPIO API from  to the
>GPIO descriptor API in  and look up GPIO
>lines from device tree or ACPI.
> +* Make pwm.c use the struct pwm_ops::apply instead of ::config, 
> ::set_polarity,
> +  ::enable and ::disable.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: audio: Fix possible leak free widgets in gbaudio_dapm_free_controls

[Johan Hovold]

On Sat, Dec 05, 2020 at 06:38:27PM +0800, Wang Hai wrote:
> In gbaudio_dapm_free_controls(), if one of the widgets is not found, an error
> will be returned directly, which will cause the rest to be unable to be freed,
> resulting in leak.
> 
> This patch fixes the bug. If if one of them is not found, just skip and free 
> the others.

Apart from the typo, please break your lines at 72 columns or so (not
needed for the Fixes tag).

> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic 
> audio module")
> Reported-by: Hulk Robot 
> Signed-off-by: Wang Hai 
> ---
>  drivers/staging/greybus/audio_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/audio_helper.c 
> b/drivers/staging/greybus/audio_helper.c
> index 237531ba60f3..3011b8abce38 100644
> --- a/drivers/staging/greybus/audio_helper.c
> +++ b/drivers/staging/greybus/audio_helper.c
> @@ -135,7 +135,8 @@ int gbaudio_dapm_free_controls(struct 
> snd_soc_dapm_context *dapm,
>   if (!w) {
>   dev_err(dapm->dev, "%s: widget not found\n",
>   widget->name);
> - return -EINVAL;
> + widget++;
> + continue;
>   }
>   widget++;
>  #ifdef CONFIG_DEBUG_FS

Not sure if we can ever have the widget lookup fail, but at least this
looks consistent now.

Reviewed-by: Johan Hovold 

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: audio: Add missing unlock in gbaudio_dapm_free_controls()

[Johan Hovold]

On Fri, Dec 04, 2020 at 05:19:25PM +0800, wanghai (M) wrote:
> 
> 在 2020/12/4 16:40, Johan Hovold 写道:
> > On Fri, Dec 04, 2020 at 10:13:50AM +0800, Wang Hai wrote:
> >> Add the missing unlock before return from function
> >> gbaudio_dapm_free_controls() in the error handling case.
> >>
> >> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic 
> >> audio module")
> >> Reported-by: Hulk Robot 
> >> Signed-off-by: Wang Hai 
> >> ---
> >>   drivers/staging/greybus/audio_helper.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/staging/greybus/audio_helper.c 
> >> b/drivers/staging/greybus/audio_helper.c
> >> index 237531ba60f3..293675dbea10 100644
> >> --- a/drivers/staging/greybus/audio_helper.c
> >> +++ b/drivers/staging/greybus/audio_helper.c
> >> @@ -135,6 +135,7 @@ int gbaudio_dapm_free_controls(struct 
> >> snd_soc_dapm_context *dapm,
> >>if (!w) {
> >>dev_err(dapm->dev, "%s: widget not found\n",
> >>widget->name);
> >> +  mutex_unlock(>card->dapm_mutex);
> >>return -EINVAL;
> >>}
> >>widget++;
> > This superficially looks correct, but there seems to be another bug in
> > this function. It can be used free an array of widgets, but if one of
> > them isn't found we just leak the rest. Perhaps that return should
> > rather be "widget++; continue;".
> >
> I think this is a good idea, should I send a v2 patch?

Let's just wait a bit and see what Vaibhav or Mark says first.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: audio: Add missing unlock in gbaudio_dapm_free_controls()

[Johan Hovold]

On Fri, Dec 04, 2020 at 10:13:50AM +0800, Wang Hai wrote:
> Add the missing unlock before return from function
> gbaudio_dapm_free_controls() in the error handling case.
> 
> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic 
> audio module")
> Reported-by: Hulk Robot 
> Signed-off-by: Wang Hai 
> ---
>  drivers/staging/greybus/audio_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/greybus/audio_helper.c 
> b/drivers/staging/greybus/audio_helper.c
> index 237531ba60f3..293675dbea10 100644
> --- a/drivers/staging/greybus/audio_helper.c
> +++ b/drivers/staging/greybus/audio_helper.c
> @@ -135,6 +135,7 @@ int gbaudio_dapm_free_controls(struct 
> snd_soc_dapm_context *dapm,
>   if (!w) {
>   dev_err(dapm->dev, "%s: widget not found\n",
>   widget->name);
> + mutex_unlock(>card->dapm_mutex);
>   return -EINVAL;
>   }
>   widget++;

This superficially looks correct, but there seems to be another bug in
this function. It can be used free an array of widgets, but if one of
them isn't found we just leak the rest. Perhaps that return should
rather be "widget++; continue;".

Vaibhav?

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/5] iio: light: lm3533-als: remove explicit parent assignment

[Johan Hovold]

On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean wrote:
> This assignment is the more peculiar of the bunch as it assigns the parent
> of the platform-device's device (i.e. pdev->dev.parent) as the IIO device's
> parent.
>
> It's unclear whether this is intentional or not.
> Hence it is in it's own patch.

Yeah, we have a few mfd drivers whose child drivers registers their
class devices directly under the parent mfd device rather than the
corresponding child platform device.

Since it's done consistently I think you need to update them all if you
really want to change this. 

And it may not be worth it since at least in theory someone could now be
relying on this topology.

> Signed-off-by: Alexandru Ardelean 
> ---
>  drivers/iio/light/lm3533-als.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index bc196c212881..0f380ec8d30c 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct platform_device *pdev)
>   indio_dev->channels = lm3533_als_channels;
>   indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
>   indio_dev->name = dev_name(>dev);
> - indio_dev->dev.parent = pdev->dev.parent;
>   indio_dev->modes = INDIO_DIRECT_MODE;
>  
>   als = iio_priv(indio_dev);

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: greybus: uart: replace driver line-coding struct

[Johan Hovold]

Drop the driver version of the line-coding request and use the protocol
definition directly as was originally intended instead.

This specifically avoids having the two versions of what is supposed to
be the same struct ever getting out of sync.

Note that this has in fact already happened once when the protocol
definition had its implicit padding removed while the driver struct
wasn't updated. The fact that we used the size of the then larger driver
struct when memcpying its content to the stack didn't exactly make
things better. A later addition of a flow-control field incidentally
made the structures match again.

Signed-off-by: Johan Hovold 
---
 drivers/staging/greybus/uart.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index 55c51143bb09..84de56800a21 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -40,14 +40,6 @@
 #define GB_UART_FIRMWARE_CREDITS   4096
 #define GB_UART_CREDIT_WAIT_TIMEOUT_MSEC   1
 
-struct gb_tty_line_coding {
-   __le32  rate;
-   __u8format;
-   __u8parity;
-   __u8data_bits;
-   __u8flow_control;
-};
-
 struct gb_tty {
struct gbphy_device *gbphy_dev;
struct tty_port port;
@@ -66,7 +58,7 @@ struct gb_tty {
struct mutex mutex;
u8 ctrlin;  /* input control lines */
u8 ctrlout; /* output control lines */
-   struct gb_tty_line_coding line_coding;
+   struct gb_uart_set_line_coding_request line_coding;
struct work_struct tx_work;
struct kfifo write_fifo;
bool close_pending;
@@ -288,12 +280,9 @@ static void  gb_uart_tx_write_work(struct work_struct 
*work)
 
 static int send_line_coding(struct gb_tty *tty)
 {
-   struct gb_uart_set_line_coding_request request;
-
-   memcpy(, >line_coding,
-  sizeof(tty->line_coding));
return gb_operation_sync(tty->connection, GB_UART_TYPE_SET_LINE_CODING,
-, sizeof(request), NULL, 0);
+>line_coding, sizeof(tty->line_coding),
+NULL, 0);
 }
 
 static int send_control(struct gb_tty *gb_tty, u8 control)
@@ -493,9 +482,9 @@ static int gb_tty_break_ctl(struct tty_struct *tty, int 
state)
 static void gb_tty_set_termios(struct tty_struct *tty,
   struct ktermios *termios_old)
 {
+   struct gb_uart_set_line_coding_request newline;
struct gb_tty *gb_tty = tty->driver_data;
struct ktermios *termios = >termios;
-   struct gb_tty_line_coding newline;
u8 newctrl = gb_tty->ctrlout;
 
newline.rate = cpu_to_le32(tty_get_baud_rate(tty));
-- 
2.26.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [greybus-dev] [PATCH] greybus: uart: fix uninitialized flow control variable

[Johan Hovold]

On Wed, Apr 29, 2020 at 03:00:44PM -0500, Alex Elder wrote:
> On 4/29/20 2:00 PM, Arnd Bergmann wrote:
> > gcc-10 points out an uninitialized variable use:
> 
> Wow, nice, checking individual uninitialized fields within
> the structure.
> 
> The structure should really be zero-initialized anyway; it's
> passed as a structure in a message elsewhere.  With your
> change, all fields in the structure are written, but in
> theory the structure could change and stack garbage could
> be sent over the wire.
> 
> What do you think of doing this instead?  Or in addition?
> 
> struct gb_tty_line_coding newline = { };
> 
> (Presumably that would also silence the warning.)
> 
> I endorse of your change, either way.

Looks like Greg ended up applying an identical version of this patch
that was submitted this week instead.

Taking a closer look at this code I noticed we have two versions of this
line-coding struct which are supposed by be identical, but which could
get out of sync (and have once already it turns out).

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Thu, Mar 12, 2020 at 05:51:11PM +0300, Dan Carpenter wrote:
> On Wed, Mar 11, 2020 at 10:58:14AM +0100, Johan Hovold wrote:
> > On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
> > > Since snprintf() returns the would-be-output size instead of the
> > > actual output size, the succeeding calls may go beyond the given
> > > buffer limit.  Fix it by replacing with scnprintf().
> > > 
> > > Signed-off-by: Takashi Iwai 
> > > ---
> > >  drivers/staging/greybus/tools/loopback_test.c | 24 
> > > 
> > 
> > Thanks for the fix.
> > 
> > Would you mind resending with a "staging: greybus: loopback_test:"
> > prefix since this is not a subsystem wide issue, bur rather a bug in a
> > specific user-space tool?
> 
> I'm surprised that user-space even has scnprintf().

Yeah, see the rest of the thread.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] staging: greybus: loopback_test: fix potential path truncations

[Johan Hovold]

Newer GCC warns about possible truncations of two generated path names as
we're concatenating the configurable sysfs and debugfs path prefixes
with a filename and placing the results in buffers of the same size as
the maximum length of the prefixes.

snprintf(d->name, MAX_STR_LEN, "gb_loopback%u", dev_id);

snprintf(d->sysfs_entry, MAX_SYSFS_PATH, "%s%s/",
 t->sysfs_prefix, d->name);

snprintf(d->debugfs_entry, MAX_SYSFS_PATH, "%sraw_latency_%s",
 t->debugfs_prefix, d->name);

Fix this by separating the maximum path length from the maximum prefix
length and reducing the latter enough to fit the generated strings.

Note that we also need to reduce the device-name buffer size as GCC
isn't smart enough to figure out that we ever only used MAX_STR_LEN
bytes of it.

Fixes: 6b0658f68786 ("greybus: tools: Add tools directory to greybus repo and 
add loopback")
Signed-off-by: Johan Hovold 
---
 drivers/staging/greybus/tools/loopback_test.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/greybus/tools/loopback_test.c 
b/drivers/staging/greybus/tools/loopback_test.c
index d38bb4fbd6b9..69c6dce9be31 100644
--- a/drivers/staging/greybus/tools/loopback_test.c
+++ b/drivers/staging/greybus/tools/loopback_test.c
@@ -19,6 +19,7 @@
 #include 
 
 #define MAX_NUM_DEVICES 10
+#define MAX_SYSFS_PREFIX 0x80
 #define MAX_SYSFS_PATH 0x200
 #define CSV_MAX_LINE   0x1000
 #define SYSFS_MAX_INT  0x20
@@ -67,7 +68,7 @@ struct loopback_results {
 };
 
 struct loopback_device {
-   char name[MAX_SYSFS_PATH];
+   char name[MAX_STR_LEN];
char sysfs_entry[MAX_SYSFS_PATH];
char debugfs_entry[MAX_SYSFS_PATH];
struct loopback_results results;
@@ -93,8 +94,8 @@ struct loopback_test {
int stop_all;
int poll_count;
char test_name[MAX_STR_LEN];
-   char sysfs_prefix[MAX_SYSFS_PATH];
-   char debugfs_prefix[MAX_SYSFS_PATH];
+   char sysfs_prefix[MAX_SYSFS_PREFIX];
+   char debugfs_prefix[MAX_SYSFS_PREFIX];
struct timespec poll_timeout;
struct loopback_device devices[MAX_NUM_DEVICES];
struct loopback_results aggregate_results;
@@ -907,10 +908,10 @@ int main(int argc, char *argv[])
t.iteration_max = atoi(optarg);
break;
case 'S':
-   snprintf(t.sysfs_prefix, MAX_SYSFS_PATH, "%s", optarg);
+   snprintf(t.sysfs_prefix, MAX_SYSFS_PREFIX, "%s", 
optarg);
break;
case 'D':
-   snprintf(t.debugfs_prefix, MAX_SYSFS_PATH, "%s", 
optarg);
+   snprintf(t.debugfs_prefix, MAX_SYSFS_PREFIX, "%s", 
optarg);
break;
case 'm':
t.mask = atol(optarg);
@@ -961,10 +962,10 @@ int main(int argc, char *argv[])
}
 
if (!strcmp(t.sysfs_prefix, ""))
-   snprintf(t.sysfs_prefix, MAX_SYSFS_PATH, "%s", sysfs_prefix);
+   snprintf(t.sysfs_prefix, MAX_SYSFS_PREFIX, "%s", sysfs_prefix);
 
if (!strcmp(t.debugfs_prefix, ""))
-   snprintf(t.debugfs_prefix, MAX_SYSFS_PATH, "%s", 
debugfs_prefix);
+   snprintf(t.debugfs_prefix, MAX_SYSFS_PREFIX, "%s", 
debugfs_prefix);
 
ret = find_loopback_devices();
if (ret)
-- 
2.24.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging: greybus: loopback_test: fix potential path truncation

[Johan Hovold]

Newer GCC warns about a possible truncation of a generated sysfs path
name as we're concatenating a directory path with a file name and
placing the result in a buffer that is half the size of the maximum
length of the directory path (which is user controlled).

loopback_test.c: In function 'open_poll_files':
loopback_test.c:651:31: warning: '%s' directive output may be truncated writing 
up to 511 bytes into a region of size 255 [-Wformat-truncation=]
  651 |   snprintf(buf, sizeof(buf), "%s%s", dev->sysfs_entry, 
"iteration_count");
  |   ^~
loopback_test.c:651:3: note: 'snprintf' output between 16 and 527 bytes into a 
destination of size 255
  651 |   snprintf(buf, sizeof(buf), "%s%s", dev->sysfs_entry, 
"iteration_count");
  |   
^~~

Fix this by making sure the buffer is large enough the concatenated
strings.

Fixes: 6b0658f68786 ("greybus: tools: Add tools directory to greybus repo and 
add loopback")
Fixes: 9250c0ee2626 ("greybus: Loopback_test: use poll instead of inotify")
Signed-off-by: Johan Hovold 
---
 drivers/staging/greybus/tools/loopback_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/tools/loopback_test.c 
b/drivers/staging/greybus/tools/loopback_test.c
index 41e1820d9ac9..d38bb4fbd6b9 100644
--- a/drivers/staging/greybus/tools/loopback_test.c
+++ b/drivers/staging/greybus/tools/loopback_test.c
@@ -637,7 +637,7 @@ int find_loopback_devices(struct loopback_test *t)
 static int open_poll_files(struct loopback_test *t)
 {
struct loopback_device *dev;
-   char buf[MAX_STR_LEN];
+   char buf[MAX_SYSFS_PATH + MAX_STR_LEN];
char dummy;
int fds_idx = 0;
int i;
-- 
2.24.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: greybus: loopback_test: fix poll-mask build breakage

[Johan Hovold]

A scripted conversion from userland POLL* to kernel EPOLL* constants
mistakingly replaced the poll flags in the loopback_test tool, which
therefore no longer builds.

Fixes: a9a08845e9ac ("vfs: do bulk POLL* -> EPOLL* replacement")
Cc: stable  # 4.16
Signed-off-by: Johan Hovold 
---
 drivers/staging/greybus/tools/loopback_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/tools/loopback_test.c 
b/drivers/staging/greybus/tools/loopback_test.c
index ba6f905f26fa..41e1820d9ac9 100644
--- a/drivers/staging/greybus/tools/loopback_test.c
+++ b/drivers/staging/greybus/tools/loopback_test.c
@@ -655,7 +655,7 @@ static int open_poll_files(struct loopback_test *t)
goto err;
}
read(t->fds[fds_idx].fd, , 1);
-   t->fds[fds_idx].events = EPOLLERR|EPOLLPRI;
+   t->fds[fds_idx].events = POLLERR | POLLPRI;
t->fds[fds_idx].revents = 0;
fds_idx++;
}
@@ -748,7 +748,7 @@ static int wait_for_complete(struct loopback_test *t)
}
 
for (i = 0; i < t->poll_count; i++) {
-   if (t->fds[i].revents & EPOLLPRI) {
+   if (t->fds[i].revents & POLLPRI) {
/* Dummy read to clear the event */
read(t->fds[i].fd, , 1);
number_of_events++;
-- 
2.24.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/3] staging: greybus: loopback_test: fix build breakage

[Johan Hovold]

The loopback_test tool hasn't received much love lately. In fact, it has
failed to build for the past two years after a scripted EPOLL*
conversion.

Newer GCC also started warning for potential string truncation of
generated path names; the last two patches addresses that.

Johan


Johan Hovold (3):
  staging: greybus: loopback_test: fix poll-mask build breakage
  staging: greybus: loopback_test: fix potential path truncation
  staging: greybus: loopback_test: fix potential path truncations

 drivers/staging/greybus/tools/loopback_test.c | 21 ++-
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
2.24.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Wed, Mar 11, 2020 at 05:45:31PM +0100, Takashi Iwai wrote:
> On Wed, 11 Mar 2020 17:40:02 +0100, Johan Hovold wrote:

> > But isn't the first snprintf() in such a sequence as much a part of the
> > problem as the following ones?
> > 
> > If the first pos = snprintf(buf, limit, ...) overflows buf, then the
> > next pos += snprintf(buf, limit - pos, ...) will be called with with a
> > negative size argument (i.e. a very large unsigned value), which
> > effectively breaks the length check regardless of whether you replace it
> > with scnprintf() or not. And all later calls will similarly continue
> > writing beyond the end of buf.
> 
> Yeah, that's the possible case although most calls are fine with it
> since the limit is PAGE_SIZE or so.  This might need a bit more
> special care.

Yeah, sure, it should be fine here too currently, even if the buffer
size is defined in the caller.

> > But wait a minute. This is user-space code, so there's no scnprintf().
> > Did you not compile test this? ;P
> >
> > In fact it seems no-one has for a while. This code is just broken and
> > doesn't even compile any more. Maybe we should just drop it instead.
> 
> Bah, I'm afraid that I overlooked this point!
> 
> I've scraped over many places via a script-like work, and did the
> compile testing of the kernel, but not about tools.  If that's the
> case, sorry for the mess, feel free to drop it. 

No worries. I'll try to take a look at the other breakages tomorrow and
see if there's any point trying to salvage this at all.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND] staging: greybus: loopback_test: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Wed, Mar 11, 2020 at 11:09:59AM +0100, Johan Hovold wrote:
> On Wed, Mar 11, 2020 at 11:05:35AM +0100, Takashi Iwai wrote:
> > Since snprintf() returns the would-be-output size instead of the
> > actual output size, the succeeding calls may go beyond the given
> > buffer limit.  Fix it by replacing with scnprintf().
> > 
> > Signed-off-by: Takashi Iwai 
> > ---
> > 
> > Just corrected the subject prefix per request.
> 
> Acked-by: Johan Hovold 

I take that back, this patch should NOT be applied.

This is user-space code so using scnprintf() doesn't make sense.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Wed, Mar 11, 2020 at 12:01:26PM +0100, Takashi Iwai wrote:
> On Wed, 11 Mar 2020 11:09:03 +0100,
> Johan Hovold wrote:
> > 
> > On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote:
> > > On Wed, 11 Mar 2020 10:58:14 +0100,
> > > Johan Hovold wrote:
> > > > 
> > > > On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
> > > > > Since snprintf() returns the would-be-output size instead of the
> > > > > actual output size, the succeeding calls may go beyond the given
> > > > > buffer limit.  Fix it by replacing with scnprintf().
> > > > > 
> > > > > Signed-off-by: Takashi Iwai 
> > > > > ---
> > > > >  drivers/staging/greybus/tools/loopback_test.c | 24 
> > > > > 
> > > > 
> > > > Thanks for the fix.
> > > > 
> > > > Would you mind resending with a "staging: greybus: loopback_test:"
> > > > prefix since this is not a subsystem wide issue, bur rather a bug in a
> > > > specific user-space tool?
> > > 
> > > OK, will do that.
> > 
> > Thanks.
> > 
> > Perhaps you should replace the snprintf() at the start of the function
> > in question as well by the way.
> 
> Yeah, it's I also wonder while working on many other codes, too.
> I decided to minimize the changes at this time and concentrate only on
> the code that has a pattern like:
>pos += snprintf(buf, limit - pos, ...)

But isn't the first snprintf() in such a sequence as much a part of the
problem as the following ones?

If the first pos = snprintf(buf, limit, ...) overflows buf, then the
next pos += snprintf(buf, limit - pos, ...) will be called with with a
negative size argument (i.e. a very large unsigned value), which
effectively breaks the length check regardless of whether you replace it
with scnprintf() or not. And all later calls will similarly continue
writing beyond the end of buf.

But wait a minute. This is user-space code, so there's no scnprintf().
Did you not compile test this? ;P

In fact it seems no-one has for a while. This code is just broken and
doesn't even compile any more. Maybe we should just drop it instead.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND] staging: greybus: loopback_test: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Wed, Mar 11, 2020 at 11:05:35AM +0100, Takashi Iwai wrote:
> Since snprintf() returns the would-be-output size instead of the
> actual output size, the succeeding calls may go beyond the given
> buffer limit.  Fix it by replacing with scnprintf().
> 
> Signed-off-by: Takashi Iwai 
> ---
> 
> Just corrected the subject prefix per request.

Acked-by: Johan Hovold 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote:
> On Wed, 11 Mar 2020 10:58:14 +0100,
> Johan Hovold wrote:
> > 
> > On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
> > > Since snprintf() returns the would-be-output size instead of the
> > > actual output size, the succeeding calls may go beyond the given
> > > buffer limit.  Fix it by replacing with scnprintf().
> > > 
> > > Signed-off-by: Takashi Iwai 
> > > ---
> > >  drivers/staging/greybus/tools/loopback_test.c | 24 
> > > 
> > 
> > Thanks for the fix.
> > 
> > Would you mind resending with a "staging: greybus: loopback_test:"
> > prefix since this is not a subsystem wide issue, bur rather a bug in a
> > specific user-space tool?
> 
> OK, will do that.

Thanks.

Perhaps you should replace the snprintf() at the start of the function
in question as well by the way.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Use scnprintf() for avoiding potential buffer overflow

[Johan Hovold]

On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
> Since snprintf() returns the would-be-output size instead of the
> actual output size, the succeeding calls may go beyond the given
> buffer limit.  Fix it by replacing with scnprintf().
> 
> Signed-off-by: Takashi Iwai 
> ---
>  drivers/staging/greybus/tools/loopback_test.c | 24 

Thanks for the fix.

Would you mind resending with a "staging: greybus: loopback_test:"
prefix since this is not a subsystem wide issue, bur rather a bug in a
specific user-space tool?

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Replace zero-length array with flexible-array member

[Johan Hovold]

On Tue, Feb 11, 2020 at 03:12:19PM -0600, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
> 
> struct foo {
> int stuff;
> struct boo array[];
> };
> 
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertenly introduced[3] to the codebase from now on.
> 
> This issue was found with the help of Coccinelle.

Looks like the scripts may need to be updated. You missed at least a
couple of instances:

$ git grep '\[0\];' drivers/staging/greybus/
drivers/staging/greybus/audio_apbridgea.h:  __u8data[0];
...
drivers/staging/greybus/usb.c:  u8 buf[0];

Would you mind replacing these as well so that we really do this in one
patch per subsystem?

> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/staging/greybus/raw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
> index 838acbe84ca0..2b301b2aa107 100644
> --- a/drivers/staging/greybus/raw.c
> +++ b/drivers/staging/greybus/raw.c
> @@ -30,7 +30,7 @@ struct gb_raw {
>  struct raw_data {
>   struct list_head entry;
>   u32 len;
> - u8 data[0];
> + u8 data[];
>  };
>  
>  static struct class *raw_class;

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced.

[Johan Hovold]

On Sun, Jan 26, 2020 at 02:01:30PM +0530, Saurav Girepunje wrote:
> Fix the warning reported by cocci check.
> 
> Changes:
> 
> In queue_work fw dereference before it actually get assigned.
> move queue_work before gb_bootrom_set_timeout.

Nope. As I said yesterday, you need to verify the output of any static
checkers you use.

The code may be unnecessarily subtle, but there's no way fw can be
dereferenced before being initialised currently.

> -queue_work:
>   /* Refresh timeout */
>   if (!ret && (offset + size == fw->size))

Specifically, the second operand is never evaluated if ret is non-zero.

>   next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> - next_request = NEXT_REQ_GET_FIRMWARE;
>   
> +queue_work:
>   gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
>   
>   return ret;

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: bootrom: fix uninitialized variables

[Johan Hovold]

On Sat, Jan 25, 2020 at 02:14:03PM +0530, Saurav Girepunje wrote:
> fix uninitialized variables issue found using static code analysis tool

Which tool is that?

> (error) Uninitialized variable: offset
> (error) Uninitialized variable: size
>
> Signed-off-by: Saurav Girepunje 
> ---
>   drivers/staging/greybus/bootrom.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/bootrom.c 
> b/drivers/staging/greybus/bootrom.c
> index a8efb86..9eabeb3 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -245,7 +245,7 @@ static int gb_bootrom_get_firmware(struct gb_operation 
> *op)
>   struct gb_bootrom_get_firmware_request *firmware_request;
>   struct gb_bootrom_get_firmware_response *firmware_response;
>   struct device *dev = >connection->bundle->dev;
> - unsigned int offset, size;
> + unsigned int offset = 0, size = 0;
>   enum next_request_type next_request;
>   int ret = 0;

I think this has come up in the past, and while the code in question is
overly complicated and confuses static checkers as well as humans, it
looks correct to me.

Please make sure to verify the output of any tools before posting
patches based on them.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: rtllib_module: Fix memory leak in alloc_rtllib

[Johan Hovold]

On Sun, Dec 15, 2019 at 08:42:47PM -0600, Navid Emamdoost wrote:
> Hi Johan,
> 
> On Sun, Dec 15, 2019 at 7:23 AM Johan Hovold  wrote:
> >
> > On Sat, Dec 14, 2019 at 05:05:58PM -0600, Navid Emamdoost wrote:
> > > In the implementation of alloc_rtllib() the allocated dev is leaked in
> > > case of ieee->pHTInfo allocation failure. Release via free_netdev(dev).
> > >
> > > Fixes: 6869a11bff1d ("Staging: rtl8192e: Use !x instead of x == NULL")
> >
> > This is not the commit that introduced this issue.
> Oops! That should be  94a799425eee8
> 
> >
> > > Signed-off-by: Navid Emamdoost 
> > > ---
> > >  drivers/staging/rtl8192e/rtllib_module.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/rtl8192e/rtllib_module.c 
> > > b/drivers/staging/rtl8192e/rtllib_module.c
> > > index 64d9feee1f39..18d898714c5c 100644
> > > --- a/drivers/staging/rtl8192e/rtllib_module.c
> > > +++ b/drivers/staging/rtl8192e/rtllib_module.c
> > > @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv)
> > >
> > >   ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), 
> > > GFP_KERNEL);
> > >   if (!ieee->pHTInfo)
> > > - return NULL;
> > > + goto failed;
> >
> > And you're still leaking ieee->networks and possibly a bunch of other
> > allocations here. You need to call at least rtllib_networks_free() in
> > the error path.
> I'm not familiar with this code, but based on your hint I believe
> there should be something like free_rtllib() here, right?

Right.

> More specifically, rtllib_softmac_free() and
> lib80211_crypt_info_free() are needed along with
> rtllib_networks_free(). If you confirm that it works I can go ahead to
> prepare patch v2 with these releases.

I can't confirm anything, that's your job. ;)

You need to trace the calls and allocations made in in alloc_rtllib()
and make sure everything is released on errors. For a well-designed
subsystem and driver this should end up looking a lot like the release
function (free_rtllib()), but that's unfortunately not always the case.

Judging from a quick look at least rtllib_softmac_free() is also needed
besides rtllib_networks_free(). And you probably want
lib80211_crypt_info_free() as well for consistency even if the
corresponding init functions doesn't seem to do any allocations
currently.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: rtllib_module: Fix memory leak in alloc_rtllib

[Johan Hovold]

On Sat, Dec 14, 2019 at 05:05:58PM -0600, Navid Emamdoost wrote:
> In the implementation of alloc_rtllib() the allocated dev is leaked in
> case of ieee->pHTInfo allocation failure. Release via free_netdev(dev).
> 
> Fixes: 6869a11bff1d ("Staging: rtl8192e: Use !x instead of x == NULL")

This is not the commit that introduced this issue.

> Signed-off-by: Navid Emamdoost 
> ---
>  drivers/staging/rtl8192e/rtllib_module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_module.c 
> b/drivers/staging/rtl8192e/rtllib_module.c
> index 64d9feee1f39..18d898714c5c 100644
> --- a/drivers/staging/rtl8192e/rtllib_module.c
> +++ b/drivers/staging/rtl8192e/rtllib_module.c
> @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv)
>  
>   ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), GFP_KERNEL);
>   if (!ieee->pHTInfo)
> - return NULL;
> + goto failed;

And you're still leaking ieee->networks and possibly a bunch of other
allocations here. You need to call at least rtllib_networks_free() in
the error path.

>  
>   HTUpdateDefaultSetting(ieee);
>   HTInitializeHTInfo(ieee);

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/2] staging: fix USB altsetting bugs

[Johan Hovold]

We had quite a few drivers using the first alternate setting instead of
the current one when doing descriptor sanity checks. This is mostly an
issue on kernels with panic_on_warn set due to a WARN() in
usb_submit_urb(), but since we've started backporting such fixes (e.g.
as reported by syzbot), I've marked these for stable as well.

Johan


Johan Hovold (2):
  staging: rtl8188eu: fix interface sanity check
  staging: rtl8712: fix interface sanity check

 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 2 +-
 drivers/staging/rtl8712/usb_intf.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: rtl8712: fix interface sanity check

[Johan Hovold]

Make sure to use the current alternate setting when verifying the
interface descriptors to avoid binding to an invalid interface.

Failing to do so could cause the driver to misbehave or trigger a WARN()
in usb_submit_urb() that kernels with panic_on_warn set would choke on.

Fixes: 2865d42c78a9 ("staging: r8712u: Add the new driver to the mainline 
kernel")
Cc: stable  # 2.6.37
Signed-off-by: Johan Hovold 
---
 drivers/staging/rtl8712/usb_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/usb_intf.c 
b/drivers/staging/rtl8712/usb_intf.c
index ba1288297ee4..a87562f632a7 100644
--- a/drivers/staging/rtl8712/usb_intf.c
+++ b/drivers/staging/rtl8712/usb_intf.c
@@ -247,7 +247,7 @@ static uint r8712_usb_dvobj_init(struct _adapter *padapter)
 
pdvobjpriv->padapter = padapter;
padapter->eeprom_address_size = 6;
-   phost_iface = >altsetting[0];
+   phost_iface = pintf->cur_altsetting;
piface_desc = _iface->desc;
pdvobjpriv->nr_endpoint = piface_desc->bNumEndpoints;
if (pusbd->speed == USB_SPEED_HIGH) {
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: rtl8188eu: fix interface sanity check

[Johan Hovold]

Make sure to use the current alternate setting when verifying the
interface descriptors to avoid binding to an invalid interface.

Failing to do so could cause the driver to misbehave or trigger a WARN()
in usb_submit_urb() that kernels with panic_on_warn set would choke on.

Fixes: c2478d39076b ("staging: r8188eu: Add files for new driver - part 20")
Cc: stable  # 3.12
Signed-off-by: Johan Hovold 
---
 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c 
b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 4fac9dca798e..a7cac0719b8b 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -70,7 +70,7 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface 
*usb_intf)
phost_conf = pusbd->actconfig;
pconf_desc = _conf->desc;
 
-   phost_iface = _intf->altsetting[0];
+   phost_iface = usb_intf->cur_altsetting;
piface_desc = _iface->desc;
 
pdvobjpriv->NumInterfaces = pconf_desc->bNumInterfaces;
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/3] staging: gigaset: fix general protection fault on probe

[Johan Hovold]

Fix a general protection fault when accessing the endpoint descriptors
which could be triggered by a malicious device due to missing sanity
checks on the number of endpoints.

Reported-by: syzbot+35b1c403a14f5c89e...@syzkaller.appspotmail.com
Fixes: 07dc1f9f2f80 ("[PATCH] isdn4linux: Siemens Gigaset drivers - M105 USB 
DECT adapter")
Cc: stable  # 2.6.17
Cc: Hansjoerg Lipp 
Cc: Tilman Schmidt 
Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index 1b9b43659bdf..5e393e7dde45 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -685,6 +685,11 @@ static int gigaset_probe(struct usb_interface *interface,
return -ENODEV;
}
 
+   if (hostif->desc.bNumEndpoints < 2) {
+   dev_err(>dev, "missing endpoints\n");
+   return -ENODEV;
+   }
+
dev_info(>dev, "%s: Device matched ... !\n", __func__);
 
/* allocate memory for our device state and initialize it */
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/3] staging: gigaset: add endpoint-type sanity check

[Johan Hovold]

Add missing endpoint-type sanity checks to probe.

This specifically prevents a warning in USB core on URB submission when
fuzzing USB descriptors.

Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index a84722d83bc6..a20c0bfa68f3 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -705,6 +705,12 @@ static int gigaset_probe(struct usb_interface *interface,
 
endpoint = >endpoint[0].desc;
 
+   if (!usb_endpoint_is_bulk_out(endpoint)) {
+   dev_err(>dev, "missing bulk-out endpoint\n");
+   retval = -ENODEV;
+   goto error;
+   }
+
buffer_size = le16_to_cpu(endpoint->wMaxPacketSize);
ucs->bulk_out_size = buffer_size;
ucs->bulk_out_epnum = usb_endpoint_num(endpoint);
@@ -724,6 +730,12 @@ static int gigaset_probe(struct usb_interface *interface,
 
endpoint = >endpoint[1].desc;
 
+   if (!usb_endpoint_is_int_in(endpoint)) {
+   dev_err(>dev, "missing int-in endpoint\n");
+   retval = -ENODEV;
+   goto error;
+   }
+
ucs->busy = 0;
 
ucs->read_urb = usb_alloc_urb(0, GFP_KERNEL);
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/3] staging: gigaset: fix illegal free on probe errors

[Johan Hovold]

The driver failed to initialise its receive-buffer pointer, something
which could lead to an illegal free on late probe errors.

Fix this by making sure to clear all driver data at allocation.

Fixes: 2032e2c2309d ("usb_gigaset: code cleanup")
Cc: stable  # 2.6.33
Cc: Tilman Schmidt 
Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index 5e393e7dde45..a84722d83bc6 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -571,8 +571,7 @@ static int gigaset_initcshw(struct cardstate *cs)
 {
struct usb_cardstate *ucs;
 
-   cs->hw.usb = ucs =
-   kmalloc(sizeof(struct usb_cardstate), GFP_KERNEL);
+   cs->hw.usb = ucs = kzalloc(sizeof(struct usb_cardstate), GFP_KERNEL);
if (!ucs) {
pr_err("out of memory\n");
return -ENOMEM;
@@ -584,9 +583,6 @@ static int gigaset_initcshw(struct cardstate *cs)
ucs->bchars[3] = 0;
ucs->bchars[4] = 0x11;
ucs->bchars[5] = 0x13;
-   ucs->bulk_out_buffer = NULL;
-   ucs->bulk_out_urb = NULL;
-   ucs->read_urb = NULL;
tasklet_init(>write_tasklet,
 gigaset_modem_fill, (unsigned long) cs);
 
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 0/3] staging: gigaset: fix crashes on probe

[Johan Hovold]

Syzbot has been reporting a GPF on probe in the gigaset ISDN driver,
which have since been moved to staging.

The first patch fixes that issue, and the next one fixes a second crash
found during testing.

The third patch addresses a benign warning in USB core which syzbot is
bound to report once the crashes have been fixed.

Johan


Changes in v2
 - use usb_endpoint_is_bulk_out() and friends in patch 3/3, and drop
   patch 4/4 which only renamed an identifier.


Johan Hovold (3):
  staging: gigaset: fix general protection fault on probe
  staging: gigaset: fix illegal free on probe errors
  staging: gigaset: add endpoint-type sanity check

 drivers/staging/isdn/gigaset/usb-gigaset.c | 23 +-
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/4] staging: gigaset: fix general protection fault on probe

[Johan Hovold]

On Sun, Dec 01, 2019 at 01:41:56PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Dec 01, 2019 at 01:30:42PM +0100, Tilman Schmidt wrote:
> > Hi Johan,
> > 
> > this is probably caused by the move of the driver to staging in
> > kernel release 5.3 half a year ago. If you want your patches to
> > apply to pre-5.3 stable releases you'll have to submit a version
> > with the paths changed from drivers/staging/isdn/gigaset to
> > drivers/isdn/gigaset.
> 
> That's trivial for me to do when they get added to the stable tree(s),
> no need to worry about it.

I'll be sending a v2 of this series shortly. Somehow I managed to
overlook usb_endpoint_is_bulk_in() and friends so patch 4/4 should no
longer be needed either.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] staging: gigaset: fix crashes on probe

[Johan Hovold]

On Fri, Nov 29, 2019 at 02:32:39PM +0100, Greg Kroah-Hartman wrote: > On Fri, 
Nov 29, 2019 at 11:17:49AM +0100, Johan Hovold wrote:
> > Syzbot has been reporting a GPF on probe in the gigaset ISDN driver,
> > which have since been moved to staging.
> > 
> > The first patch fixes that issue, and the next one fixes a second crash
> > found during testing.
> > 
> > The third patch addresses a benign warning in USB core which syzbot is
> > bound to report once the crashes have been fixed.
> > 
> > And while I hate playing checkpatch games, the final patch addresses a
> > checkpatch warning introduced on purpose by the third patch.
> 
> I'll take these after 5.5-rc1, but then it is time to just delete all of
> drivers/staging/isdn/ from my tree, so don't worry about them after that
> :)

Sounds good to me. :)

But we should probably get these backported before dropping
staging/isdn. Not sure if syzbot is run against older stable trees as
well, but if so, you may want to consider adding a stable-tag also to
patch 3/4.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] staging: gigaset: rename endpoint-descriptor identifier

[Johan Hovold]

Rename an endpoint-descriptor pointer to shut up a checkpatch warning
about a line being over 80 columns, which is bound to generate a bunch
of clean up patches otherwise.

Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index 6c07c8379711..d5fab2ea25b4 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -652,7 +652,7 @@ static int gigaset_probe(struct usb_interface *interface,
struct usb_host_interface *hostif = interface->cur_altsetting;
struct cardstate *cs = NULL;
struct usb_cardstate *ucs = NULL;
-   struct usb_endpoint_descriptor *endpoint;
+   struct usb_endpoint_descriptor *epd;
int buffer_size;
 
gig_dbg(DEBUG_ANY, "%s: Check if device matches ...", __func__);
@@ -703,17 +703,17 @@ static int gigaset_probe(struct usb_interface *interface,
/* save address of controller structure */
usb_set_intfdata(interface, cs);
 
-   endpoint = >endpoint[0].desc;
+   epd = >endpoint[0].desc;
 
-   if (!usb_endpoint_dir_out(endpoint) || 
!usb_endpoint_xfer_bulk(endpoint)) {
+   if (!usb_endpoint_dir_out(epd) || !usb_endpoint_xfer_bulk(epd)) {
dev_err(>dev, "missing bulk-out endpoint\n");
retval = -ENODEV;
goto error;
}
 
-   buffer_size = le16_to_cpu(endpoint->wMaxPacketSize);
+   buffer_size = le16_to_cpu(epd->wMaxPacketSize);
ucs->bulk_out_size = buffer_size;
-   ucs->bulk_out_epnum = usb_endpoint_num(endpoint);
+   ucs->bulk_out_epnum = usb_endpoint_num(epd);
ucs->bulk_out_buffer = kmalloc(buffer_size, GFP_KERNEL);
if (!ucs->bulk_out_buffer) {
dev_err(cs->dev, "Couldn't allocate bulk_out_buffer\n");
@@ -728,9 +728,9 @@ static int gigaset_probe(struct usb_interface *interface,
goto error;
}
 
-   endpoint = >endpoint[1].desc;
+   epd = >endpoint[1].desc;
 
-   if (!usb_endpoint_dir_in(endpoint) || !usb_endpoint_xfer_int(endpoint)) 
{
+   if (!usb_endpoint_dir_in(epd) || !usb_endpoint_xfer_int(epd)) {
dev_err(>dev, "missing int-in endpoint\n");
retval = -ENODEV;
goto error;
@@ -744,7 +744,7 @@ static int gigaset_probe(struct usb_interface *interface,
retval = -ENOMEM;
goto error;
}
-   buffer_size = le16_to_cpu(endpoint->wMaxPacketSize);
+   buffer_size = le16_to_cpu(epd->wMaxPacketSize);
ucs->rcvbuf_size = buffer_size;
ucs->rcvbuf = kmalloc(buffer_size, GFP_KERNEL);
if (!ucs->rcvbuf) {
@@ -754,10 +754,10 @@ static int gigaset_probe(struct usb_interface *interface,
}
/* Fill the interrupt urb and send it to the core */
usb_fill_int_urb(ucs->read_urb, udev,
-usb_rcvintpipe(udev, usb_endpoint_num(endpoint)),
+usb_rcvintpipe(udev, usb_endpoint_num(epd)),
 ucs->rcvbuf, buffer_size,
 gigaset_read_int_callback,
-cs, endpoint->bInterval);
+cs, epd->bInterval);
 
retval = usb_submit_urb(ucs->read_urb, GFP_KERNEL);
if (retval) {
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/4] staging: gigaset: add endpoint-type sanity check

[Johan Hovold]

Add endpoint type-sanity checks to prevent a warning in USB core on URB
submission.

Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index a84722d83bc6..6c07c8379711 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -705,6 +705,12 @@ static int gigaset_probe(struct usb_interface *interface,
 
endpoint = >endpoint[0].desc;
 
+   if (!usb_endpoint_dir_out(endpoint) || 
!usb_endpoint_xfer_bulk(endpoint)) {
+   dev_err(>dev, "missing bulk-out endpoint\n");
+   retval = -ENODEV;
+   goto error;
+   }
+
buffer_size = le16_to_cpu(endpoint->wMaxPacketSize);
ucs->bulk_out_size = buffer_size;
ucs->bulk_out_epnum = usb_endpoint_num(endpoint);
@@ -724,6 +730,12 @@ static int gigaset_probe(struct usb_interface *interface,
 
endpoint = >endpoint[1].desc;
 
+   if (!usb_endpoint_dir_in(endpoint) || !usb_endpoint_xfer_int(endpoint)) 
{
+   dev_err(>dev, "missing int-in endpoint\n");
+   retval = -ENODEV;
+   goto error;
+   }
+
ucs->busy = 0;
 
ucs->read_urb = usb_alloc_urb(0, GFP_KERNEL);
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] staging: gigaset: fix illegal free on probe errors

[Johan Hovold]

The driver failed to initialise its receive-buffer pointer, something
which could lead to an illegal free on late probe errors.

Fix this by making sure to clear all driver data at allocation.

Fixes: 2032e2c2309d ("usb_gigaset: code cleanup")
Cc: stable  # 2.6.33
Cc: Tilman Schmidt 
Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index 5e393e7dde45..a84722d83bc6 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -571,8 +571,7 @@ static int gigaset_initcshw(struct cardstate *cs)
 {
struct usb_cardstate *ucs;
 
-   cs->hw.usb = ucs =
-   kmalloc(sizeof(struct usb_cardstate), GFP_KERNEL);
+   cs->hw.usb = ucs = kzalloc(sizeof(struct usb_cardstate), GFP_KERNEL);
if (!ucs) {
pr_err("out of memory\n");
return -ENOMEM;
@@ -584,9 +583,6 @@ static int gigaset_initcshw(struct cardstate *cs)
ucs->bchars[3] = 0;
ucs->bchars[4] = 0x11;
ucs->bchars[5] = 0x13;
-   ucs->bulk_out_buffer = NULL;
-   ucs->bulk_out_urb = NULL;
-   ucs->read_urb = NULL;
tasklet_init(>write_tasklet,
 gigaset_modem_fill, (unsigned long) cs);
 
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/4] staging: gigaset: fix general protection fault on probe

[Johan Hovold]

Fix a general protection fault when accessing the endpoint descriptors
which could be triggered by a malicious device due to missing sanity
checks on the number of endpoints.

Reported-by: syzbot+35b1c403a14f5c89e...@syzkaller.appspotmail.com
Fixes: 07dc1f9f2f80 ("[PATCH] isdn4linux: Siemens Gigaset drivers - M105 USB 
DECT adapter")
Cc: stable  # 2.6.17
Cc: Hansjoerg Lipp 
Cc: Tilman Schmidt 
Signed-off-by: Johan Hovold 
---
 drivers/staging/isdn/gigaset/usb-gigaset.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/isdn/gigaset/usb-gigaset.c 
b/drivers/staging/isdn/gigaset/usb-gigaset.c
index 1b9b43659bdf..5e393e7dde45 100644
--- a/drivers/staging/isdn/gigaset/usb-gigaset.c
+++ b/drivers/staging/isdn/gigaset/usb-gigaset.c
@@ -685,6 +685,11 @@ static int gigaset_probe(struct usb_interface *interface,
return -ENODEV;
}
 
+   if (hostif->desc.bNumEndpoints < 2) {
+   dev_err(>dev, "missing endpoints\n");
+   return -ENODEV;
+   }
+
dev_info(>dev, "%s: Device matched ... !\n", __func__);
 
/* allocate memory for our device state and initialize it */
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/4] staging: gigaset: fix crashes on probe

[Johan Hovold]

Syzbot has been reporting a GPF on probe in the gigaset ISDN driver,
which have since been moved to staging.

The first patch fixes that issue, and the next one fixes a second crash
found during testing.

The third patch addresses a benign warning in USB core which syzbot is
bound to report once the crashes have been fixed.

And while I hate playing checkpatch games, the final patch addresses a
checkpatch warning introduced on purpose by the third patch.

Johan


Johan Hovold (4):
  staging: gigaset: fix general protection fault on probe
  staging: gigaset: fix illegal free on probe errors
  staging: gigaset: add endpoint-type sanity check
  staging: gigaset: rename endpoint-descriptor identifier

 drivers/staging/isdn/gigaset/usb-gigaset.c | 39 ++
 1 file changed, 26 insertions(+), 13 deletions(-)

-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: loopback_test: remove multiple blank lines

[Johan Hovold]

On Wed, Sep 04, 2019 at 09:05:47PM +, Pedro Chinen wrote:
> Fix following checkpath warnings in multiple lines:
> CHECK: Please don't use multiple blank lines

Checkpatch reports five instances of this CHECK, please fix them all in
one go.

> Signed-off-by: Pedro Chinen 
> ---
>  drivers/staging/greybus/tools/loopback_test.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/tools/loopback_test.c 
> b/drivers/staging/greybus/tools/loopback_test.c
> index ba6f905f26fa..1e78c148d7cb 100644
> --- a/drivers/staging/greybus/tools/loopback_test.c
> +++ b/drivers/staging/greybus/tools/loopback_test.c
> @@ -779,7 +779,6 @@ static void prepare_devices(struct loopback_test *t)
>   if (t->stop_all || device_enabled(t, i))
>   write_sysfs_val(t->devices[i].sysfs_entry, "type", 0);
>  
> -
>   for (i = 0; i < t->device_count; i++) {
>   if (!device_enabled(t, i))
>   continue;
> @@ -850,7 +849,6 @@ void loopback_run(struct loopback_test *t)
>   if (ret)
>   goto err;
>  
> -
>   get_results(t);
>  
>   log_results(t);
> @@ -882,7 +880,6 @@ static int sanity_check(struct loopback_test *t)
>  
>   }
>  
> -
>   return 0;
>  }

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: Adding missing brackets into if..else block.

[Johan Hovold]

On Wed, Sep 04, 2019 at 08:32:09PM +, Julio Faracco wrote:
> Inside a block of if..else conditional, else structure does not contain
> brackets. This is not following regular policies of good coding style.

s/good/kernel/ ?

> All parts of this conditional blocks should respect brackets inclusion.

Did you check that there aren't further instances of this style
issue in this file? If so, please them all in one go.

Also please include greybus component you are changing in your subject
prefix:

staging: greybus: loopback_test: ...

> Signed-off-by: Julio Faracco 
> ---
>  drivers/staging/greybus/tools/loopback_test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/tools/loopback_test.c 
> b/drivers/staging/greybus/tools/loopback_test.c
> index ba6f905f2..d46721502 100644
> --- a/drivers/staging/greybus/tools/loopback_test.c
> +++ b/drivers/staging/greybus/tools/loopback_test.c
> @@ -801,8 +801,9 @@ static void prepare_devices(struct loopback_test *t)
>   write_sysfs_val(t->devices[i].sysfs_entry,
>   "outstanding_operations_max",
>   t->async_outstanding_operations);
> - } else
> + } else {
>   write_sysfs_val(t->devices[i].sysfs_entry, "async", 0);
> + }
>   }
>  }

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: remove blank line after an open brace '{'.

[Johan Hovold]

On Wed, Sep 04, 2019 at 08:55:58PM +, joahannes wrote:
> Fix checkpatch error
> "CHECK: Blank lines aren't necessary after an open brace '{'"
> in loopback_test.c:742.

Please fix up all of the blank lines before/after closing/opening brace
checkpatch CHECKs in one go. There appears to be many of them.

> Signed-off-by: joahannes 

We need your full name here and in the From line.

> ---
>  drivers/staging/greybus/tools/loopback_test.c | 1 -

Also, please include the component your modifying in the subject line
even if you need to shorten the description somehow, for example, 

staging: greybus: loopback_test: remove unnecessary blank lines

>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/tools/loopback_test.c 
> b/drivers/staging/greybus/tools/loopback_test.c
> index ba6f905f2..251b05710 100644
> --- a/drivers/staging/greybus/tools/loopback_test.c
> +++ b/drivers/staging/greybus/tools/loopback_test.c
> @@ -739,7 +739,6 @@ static int wait_for_complete(struct loopback_test *t)
>   ts = >poll_timeout;
>  
>   while (1) {
> -
>   ret = ppoll(t->fds, t->poll_count, ts, _old);
>   if (ret <= 0) {
>   stop_tests(t);

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: fix more header declarations

[Johan Hovold]

On Wed, Aug 28, 2019 at 01:48:25PM +0100, Rui Miguel Silva wrote:
> More headers needed to be fixed when moving greybus out of staging and
> enabling the COMPILE_TEST option.
> 
> Add forward declarations for the needed structures.
> 
> Reported-by: kbuild test robot 
> Signed-off-by: Rui Miguel Silva 
> ---
> v1->v2:
> Johan Hovold:
>   - use forward declarations instead including all headers

Reviewed-by: Johan Hovold 

>  include/linux/greybus/operation.h | 2 +-
>  include/linux/greybus/svc.h   | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/greybus/operation.h 
> b/include/linux/greybus/operation.h
> index 8ca864bba23e..cb8e4ef45222 100644
> --- a/include/linux/greybus/operation.h
> +++ b/include/linux/greybus/operation.h
> @@ -15,7 +15,7 @@
>  #include 
>  #include 
>  
> -
> +struct gb_host_device;
>  struct gb_operation;
>  
>  /* The default amount of time a request is given to complete */
> diff --git a/include/linux/greybus/svc.h b/include/linux/greybus/svc.h
> index 507f8bd4e4c8..5afaf5f06856 100644
> --- a/include/linux/greybus/svc.h
> +++ b/include/linux/greybus/svc.h
> @@ -12,6 +12,8 @@
>  #include 
>  #include 
>  
> +struct gb_svc_l2_timer_cfg;
> +
>  #define GB_SVC_CPORT_FLAG_E2EFC  BIT(0)
>  #define GB_SVC_CPORT_FLAG_CSD_N  BIT(1)
>  #define GB_SVC_CPORT_FLAG_CSV_N  BIT(2)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: fix more header includes

[Johan Hovold]

On Wed, Aug 28, 2019 at 11:28:59AM +0100, Rui Miguel Silva wrote:
> More headers needed to be fixed when moving greybus out of staging and
> enabling the COMPILE_TEST option.
> 
> Reported-by: kbuild test robot 
> Signed-off-by: Rui Miguel Silva 
> ---
>  include/linux/greybus/operation.h | 1 +
>  include/linux/greybus/svc.h   | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/linux/greybus/operation.h 
> b/include/linux/greybus/operation.h
> index 8ca864bba23e..bfbc56d8d863 100644
> --- a/include/linux/greybus/operation.h
> +++ b/include/linux/greybus/operation.h
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  
> +#include "hd.h"

No need to include hd.h, you only need a forward declaration of struct
gb_host_device.

>  struct gb_operation;
>  
> diff --git a/include/linux/greybus/svc.h b/include/linux/greybus/svc.h
> index 507f8bd4e4c8..11a86504c429 100644
> --- a/include/linux/greybus/svc.h
> +++ b/include/linux/greybus/svc.h
> @@ -12,6 +12,8 @@
>  #include 
>  #include 
>  
> +#include "greybus_protocols.h"

Same here, no need to include all the protocol definitions for struct
gb_svc_l2_timer_cfg.

> +
>  #define GB_SVC_CPORT_FLAG_E2EFC  BIT(0)
>  #define GB_SVC_CPORT_FLAG_CSD_N  BIT(1)
>  #define GB_SVC_CPORT_FLAG_CSV_N  BIT(2)

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: power_supply: use struct_size() helper

[Johan Hovold]

On Wed, Apr 17, 2019 at 01:44:40PM -0500, Gustavo A. R. Silva wrote:
> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes, in particular in the
> context in which this code is being used.
> 
> So, replace code of the following form:
> 
> sizeof(*resp) + props_count * sizeof(struct gb_power_supply_props_desc)
> 
> with:
> 
> struct_size(resp, props, props_count)
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
> Changes in v2:
>  - Rebase on top of 47830c1127ef ("staging: greybus: power_supply: fix 
> prop-descriptor request size")

Thanks for rebasing.

Reviewed-by: Johan Hovold 

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Johan Hovold]

On Wed, Apr 17, 2019 at 06:19:50AM -0500, Alex Elder wrote:

> I'm not completely sure about the inline function, but on the no blank
> lines thing (and many other minor issues) "checkpatch.pl" is to blame.
> There are lots of examples of issues that checkpatch points out that are
> matters of opinion and not hardened kernel style rules.
> 
> We try to encourage people to get involved with kernel development by
> fixing minor problems, and we tell them a good way to find them is
> by running checkpatch and "fixing" what it reports.  Unfortunately,
> it is often things of this type, and reviewers balk and say "no,
> please leave it," and the poor new person has a bad first experience.
> 
> I *like* "checkpatch.pl".  And the fact that it can point out some
> of these sorts of things is great.  But it would be nice if certain
> types of problems (like multiple blank lines, or lines that are 81
> characters wide for example) would only be reported when a "--strict"
> option or something were supplied.

The problem is that --strict is enabled by default when running
checkpatch on code in staging (and net).

And it has all sorts of weird tests (prefixed as "CHECK" rather than
"WARNING") to catch everyone and their mom's pet peeve.

I don't think the intention ever was that all those should be "fixed",
but this appears to be where this checkpatch mission creep comes from
(and we're now seeing --strict being used on code outside of staging
too).

IMO we're setting a bad example for new contributers by accepting such
changes by default. Blindly trusting a tool is not how kernel
development works, but that seems to be the message currently sent.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Johan Hovold]

On Mon, Apr 15, 2019 at 05:40:16PM +0300, Dan Carpenter wrote:
> What I'm saying is that if we just apply it then we avoid the long
> discussion forever.  The macro is OK, sure, but it's not like anyone is
> going to come back later and argue that macros are better or preferred.

That may be a valid argument, at least with respect to staging
currently (even if checkpatch isn't entirely too blame for this
particular "cleanup").

But then all eight or so instances should be replaced in one go, to
maintain consistency.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Johan Hovold]

On Mon, Apr 15, 2019 at 04:33:57PM +0300, Dan Carpenter wrote:
> On Mon, Apr 15, 2019 at 03:10:02PM +0200, Johan Hovold wrote:
> > On Fri, Apr 05, 2019 at 03:14:37PM -0500, Madhumitha Prabakaran wrote:
> > > Fix a blank line after function/struct/union/enum
> > > declarations. Also, convert to_gb_control()  macro into an inline
> > > function in order to maintain Linux kernel coding style based
> > > on which the inline function is preferable over the macro.
> > 
> > There are about 1200 macros wrapping container_of() in the kernel, so
> > this is a common pattern which does not need to be changed (by contrast,
> > only a handful container_of are wrapped by functions).
> > 
> 
> A handful?
> 
> $ git grep "return container_of" | wc -l
> 1856
> 
> I'm like Donald Trump because I must have really small hands compared
> to the rest of y'all.

Heh, you're right, my grep-fu failed me.

Ok, so both styles are roughly as common (and a non-trivial portion of
those 1800 also are not simple wrappers).

> I would prefer to apply these sorts of patches unless it causes an
> issue...  Functions *are* better than macros.  It's a minor improvement,
> but this is staging and we make tons minor style changes.  It's part of
> the blessing and curse of being in staging...

While I agree functions are generally preferred, container_of() is
special as it includes type checking.

And again, using container_of() like this is a common pattern in the
kernel.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Johan Hovold]

On Fri, Apr 05, 2019 at 03:14:37PM -0500, Madhumitha Prabakaran wrote:
> Fix a blank line after function/struct/union/enum
> declarations. Also, convert to_gb_control()  macro into an inline
> function in order to maintain Linux kernel coding style based
> on which the inline function is preferable over the macro.

There are about 1200 macros wrapping container_of() in the kernel, so
this is a common pattern which does not need to be changed (by contrast,
only a handful container_of are wrapped by functions).

> Signed-off-by: Madhumitha Prabakaran 
> ---
>  drivers/staging/greybus/control.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/control.h 
> b/drivers/staging/greybus/control.h
> index 3a29ec05f631..a681ef74e7fe 100644
> --- a/drivers/staging/greybus/control.h
> +++ b/drivers/staging/greybus/control.h
> @@ -24,7 +24,11 @@ struct gb_control {
>   char *vendor_string;
>   char *product_string;
>  };
> -#define to_gb_control(d) container_of(d, struct gb_control, dev)
> +

No need for a blank line either as the macro is closely related to the
preceding struct declaration.

> +static inline struct gb_control *to_gb_control(struct device *d)
> +{
> + return container_of(d, struct gb_control, dev);
> +}

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [greybus-dev] [PATCH] Staging: greybus: Fix spinlock_t definition without comment

[Johan Hovold]

On Fri, Apr 05, 2019 at 05:50:10PM -0500, Alex Elder wrote:
> On 4/5/19 3:53 PM, Dan Carpenter wrote:
> > On Fri, Apr 05, 2019 at 03:00:46PM -0500, Madhumitha Prabakaran
> > wrote:
> >> Fix spinlock_t definition without comment.
> >> 
> >> Signed-off-by: Madhumitha Prabakaran 
> 
> Madhumitha, the reason one would want a comment associated with
> a lock field in a structure is to get some understanding of why
> it's needed.  Saying "protect structure fields" is not enough,
> because that can pretty much be assumed, so a comment like that
> adds no value.
> 
> Looking at the code, you can see the lock field protects the
> connection's operations list.  It also appears to be needed
> for accessing the state field (reading or updating).
> 
> Given that, a better comment might be:
> 
> spinlock_t  lock; /* operations list and state */
> 
> >> --- drivers/staging/greybus/connection.h | 2 +- 1 file changed, 1
> >> insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/staging/greybus/connection.h
> >> b/drivers/staging/greybus/connection.h index
> >> 5ca3befc0636..0aedd246e94a 100644 ---
> >> a/drivers/staging/greybus/connection.h +++
> >> b/drivers/staging/greybus/connection.h @@ -47,7 +47,7 @@ struct
> >> gb_connection { unsigned long  flags;
> >> 
> >> struct mutex   mutex; -spinlock_t  
> >> lock; + spinlock_t  lock; /*
> >> Protect structure fields */ enum gb_connection_state   state;
> > 
> > What does the mutex do then?  Why can't we just use the spinlock for 
> > everything?
> 
> The mutex needs to be held during enable and disable of a connection.
> Johan might be able to give you a more complete answer but these
> operations (or at least the enable) need to block, so can't hold a
> spinlock.

Yeah, I should have documented this at the time.

You're right that the connection spinlock protects the operation list,
and the mutex the connection state, but there are some other
dependencies here and I don't have time to look at this at the moment.

Better to leave as is, I'd say.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: greybus: usb: Fixed a coding style error

[Johan Hovold]

On Sun, Mar 31, 2019 at 01:30:40AM -0400, Will Cunningham wrote:
> Line was >80 characters.
> 
> Signed-off-by: Will Cunningham 
> ---
>  drivers/staging/greybus/usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> index 1c246c73a085..5b4cbec88159 100644
> --- a/drivers/staging/greybus/usb.c
> +++ b/drivers/staging/greybus/usb.c
> @@ -169,8 +169,8 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev,
>   return -ENOMEM;
>  
>   connection = gb_connection_create(gbphy_dev->bundle,
> -   
> le16_to_cpu(gbphy_dev->cport_desc->id),
> -   NULL);
> + le16_to_cpu(gbphy_dev->cport_desc->id),
> + NULL);

As others have already pointed out in this thread, there's no need to
fix anything here.

The 80 column rule is not absolute in any way, and having a line be 81
characters if that improves readability is just fine.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: power_supply: Use struct_size() helper

[Johan Hovold]

On Thu, Apr 04, 2019 at 08:09:51AM +0100, Rui Miguel Silva wrote:
> Hi Gustavo,
> Thanks a lot for the patch.
> 
> On Wed 03 Apr 2019 at 21:58, Gustavo A. R. Silva wrote:
> > Make use of the struct_size() helper instead of an open-coded 
> > version
> > in order to avoid any potential type mistakes, in particular in 
> > the
> > context in which this code is being used.
> >
> > So, replace code of the following form:
> >
> > sizeof(*resp) + props_count * sizeof(struct 
> > gb_power_supply_props_desc)
> >
> > with:
> >
> > struct_size(resp, props, props_count)
> >
> > This code was detected with the help of Coccinelle.
> >
> > Signed-off-by: Gustavo A. R. Silva 
> 
> What are the odds of 2 people changing same code in greybus in
> the same day :).

Well, I only noticed the bug in the current code, when reviewing
Gustavo's diff. ;)

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: power_supply: Use struct_size() helper

[Johan Hovold]

On Wed, Apr 03, 2019 at 03:58:01PM -0500, Gustavo A. R. Silva wrote:
> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes, in particular in the
> context in which this code is being used.
> 
> So, replace code of the following form:
> 
> sizeof(*resp) + props_count * sizeof(struct gb_power_supply_props_desc)
> 
> with:
> 
> struct_size(resp, props, props_count)
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/staging/greybus/power_supply.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/power_supply.c 
> b/drivers/staging/greybus/power_supply.c
> index 0529e5628c24..40cc2d462ba0 100644
> --- a/drivers/staging/greybus/power_supply.c
> +++ b/drivers/staging/greybus/power_supply.c
> @@ -520,8 +520,8 @@ static int gb_power_supply_prop_descriptors_get(struct 
> gb_power_supply *gbpsy)
>  
>   op = gb_operation_create(connection,
>GB_POWER_SUPPLY_TYPE_GET_PROP_DESCRIPTORS,
> -  sizeof(req), sizeof(*resp) + props_count *

This patch looks good, but I noticed a bug here in the current code,
which should be fixed before applying this clean up.

sizeof(req) should have been sizeof(*req) above.

> -  sizeof(struct gb_power_supply_props_desc),
> +  sizeof(req),
> +  struct_size(resp, props, props_count),
>GFP_KERNEL);
>   if (!op)
>   return -ENOMEM;

I've just submitted a fix (and CCed you as well). 

Would you mind respinning on top of that one?

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: greybus: power_supply: fix prop-descriptor request size

[Johan Hovold]

Since moving the message buffers off the stack, the dynamically
allocated get-prop-descriptor request buffer is incorrectly sized due to
using the pointer rather than request-struct size when creating the
operation.

Fortunately, the pointer size is always larger than this one-byte
request, but this could still cause trouble on the remote end due to the
unexpected message size.

Fixes: 9d15134d067e ("greybus: power_supply: rework get descriptors")
Cc: stable  # 4.9
Cc: Rui Miguel Silva 
Signed-off-by: Johan Hovold 
---
 drivers/staging/greybus/power_supply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/power_supply.c 
b/drivers/staging/greybus/power_supply.c
index 0529e5628c24..ae5c0285a942 100644
--- a/drivers/staging/greybus/power_supply.c
+++ b/drivers/staging/greybus/power_supply.c
@@ -520,7 +520,7 @@ static int gb_power_supply_prop_descriptors_get(struct 
gb_power_supply *gbpsy)
 
op = gb_operation_create(connection,
 GB_POWER_SUPPLY_TYPE_GET_PROP_DESCRIPTORS,
-sizeof(req), sizeof(*resp) + props_count *
+sizeof(*req), sizeof(*resp) + props_count *
 sizeof(struct gb_power_supply_props_desc),
 GFP_KERNEL);
if (!op)
-- 
2.21.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] tty: Fix WARNING in tty_set_termios

[Johan Hovold]

On Thu, Jan 31, 2019 at 04:18:33PM +0100, Marcel Holtmann wrote:

> > I agree with Al that this change doesn't make much sense. The WARN_ON
> > is there to catch any bugs leading to the termios being changed for a
> > master side pty. Those should bugs should be fixed, and not worked
> > around in order to silence a WARN_ON.
> > 
> > The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
> > operational speed during setup") which introduced a new way for how
> > tty_set_termios() could end up being called for a master pty.
> > 
> > As Al hinted at, setting these ldiscs for a master pty really makes no
> > sense and perhaps that is what we should prevent unless simply making
> > sure they do not call tty_set_termios() is sufficient for the time
> > being.
> > 
> > Finally, note that serdev never operates on a pty, and that this is only
> > an issue for (the three) line disciplines.
> 
> I think for PTYs we should just fail setting the HCI line discipline.
> Fail early and just move on with life.

Sounds good to me. At least for the pty master. There may be some people
trying to use a bluetooth device connected to a remote serial port (I've
seen descriptions of such setups at least), and maybe we need not prevent
that.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] tty: Fix WARNING in tty_set_termios

[Johan Hovold]

On Mon, Jan 28, 2019 at 02:29:22PM -0700, shuah wrote:
> On 1/25/19 9:14 PM, Al Viro wrote:
> > On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
> >> tty_set_termios() has the following WARMN_ON which can be triggered with a
> >> syscall to invoke TIOCGETD __NR_ioctl.

You meant TIOCSETD here, and in fact its the call which sets the uart
protocol that triggers the warning.

> >> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> >>  tty->driver->subtype == PTY_TYPE_MASTER);
> >> Reference: 
> >> https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
> >>
> >> A simple change would have been to print error message instead of WARN_ON.
> >> However, the callers assume that tty_set_termios() always returns 0 and
> >> don't check return value. The complete solution is fixing all the callers
> >> to check error and bail out to fix the WARN_ON.
> >>
> >> This fix changes tty_set_termios() to return error and all the callers
> >> to check error and bail out. The reproducer is used to reproduce the
> >> problem and verify the fix.
> > 
> >> --- a/drivers/bluetooth/hci_ldisc.c
> >> +++ b/drivers/bluetooth/hci_ldisc.c
> >> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, 
> >> bool enable)
> >>status = tty_set_termios(tty, );
> >>BT_DBG("Disabling hardware flow control: %s",
> >>   status ? "failed" : "success");
> >> +  if (status)
> >> +  return;
> > 
> > Can that ldisc end up set on pty master?  And does it make any sense there?
> 
> The initial objective of the patch is to prevent the WARN_ON by making
> the change to return error instead of WARN_ON. However, without changes
> to places that don't check the return and keep making progress, there
> will be secondary problems.
> 
> Without this change to return here, instead of WARN_ON, it will fail
> with the following NULL pointer dereference at the next thing 
> hci_uart_set_flow_control() attempts.
> 
> status = tty->driver->ops->tiocmget(tty);
> 
> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer 

That's a separate issue, which is being fixed:

https://lkml.kernel.org/r/20190130095938.GP3691@localhost

> > IOW, I don't believe that this patch makes any sense.  If anything,
> > we need to prevent unconditional tty_set_termios() on the path
> > that *does* lead to calling it for pty.
> 
> I don't think preventing unconditional tty_set_termios() is enough to
> prevent secondary problems such as the one above.
> 
> For example, the following call chain leads to the WARN_ON that was
> reported. Even if void hci_uart_set_baudrate() prevents the very first
> tty_set_termios() call, its caller hci_uart_setup() continues with
> more tty setup. It goes ahead to call driver setup callback. The
> driver callback goes on to do more setup calling tty_set_termios().
> 
> WARN_ON call path:
>   hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378
>   hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401
>   hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423
> 
> Once this WARN_ON is changed to return error, the following
> happens, when hci_uart_setup() does driver setup callback.
> 
> kernel: [10140.649836]  mrvl_setup+0x17/0x80 [hci_uart]
> kernel: [10140.649840]  hci_uart_setup+0x56/0x160 [hci_uart]
> kernel: [10140.649850]  hci_dev_do_open+0xe6/0x630 [bluetooth]
> kernel: [10140.649860]  hci_power_on+0x52/0x220 [bluetooth]
> 
> I think continuing to catch the invalid condition in tty_set_termios()
> and preventing progress by checking return value is a straight forward
> change to avoid secondary problems, and it might be difficult to catch
> all the cases where it could fail.

I agree with Al that this change doesn't make much sense. The WARN_ON
is there to catch any bugs leading to the termios being changed for a
master side pty. Those should bugs should be fixed, and not worked
around in order to silence a WARN_ON.

The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
operational speed during setup") which introduced a new way for how
tty_set_termios() could end up being called for a master pty.

As Al hinted at, setting these ldiscs for a master pty really makes no
sense and perhaps that is what we should prevent unless simply making
sure they do not call tty_set_termios() is sufficient for the time
being.

Finally, note that serdev never operates on a pty, and that this is only
an issue for (the three) line disciplines.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: speakup: fix tty-operation NULL derefs

[Johan Hovold]

The send_xchar() and tiocmset() tty operations are optional. Add the
missing sanity checks to prevent user-space triggerable NULL-pointer
dereferences.

Fixes: 6b9ad1c742bf ("staging: speakup: add send_xchar, tiocmset and input 
functionality for tty")
Cc: stable  # 4.13
Cc: Okash Khawaja 
Cc: Samuel Thibault 
Signed-off-by: Johan Hovold 
---
 drivers/staging/speakup/spk_ttyio.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/speakup/spk_ttyio.c 
b/drivers/staging/speakup/spk_ttyio.c
index c92bbd05516e..005de0024dd4 100644
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -265,7 +265,8 @@ static void spk_ttyio_send_xchar(char ch)
return;
}
 
-   speakup_tty->ops->send_xchar(speakup_tty, ch);
+   if (speakup_tty->ops->send_xchar)
+   speakup_tty->ops->send_xchar(speakup_tty, ch);
mutex_unlock(_tty_mutex);
 }
 
@@ -277,7 +278,8 @@ static void spk_ttyio_tiocmset(unsigned int set, unsigned 
int clear)
return;
}
 
-   speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+   if (speakup_tty->ops->tiocmset)
+   speakup_tty->ops->tiocmset(speakup_tty, set, clear);
mutex_unlock(_tty_mutex);
 }
 
-- 
2.20.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 0/3] greybus: gpio: Switch to the gpio descriptor interface

[Johan Hovold]

On Mon, Jan 14, 2019 at 08:25:07PM +0530, Nishad Kamdar wrote:
> This patch series converts uses of the old GPIO API to the GPIO
> descriptor API. It also converts the GPIO driver to use the
> GPIO irqchip library GPIOLIB_IRQCHIP instead of repimplementing
> the same.
> 
> Chnages in v7:
>  - Combine the three patches together.

Thanks for resending, all looks good now.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 0/2] greybus: gpio: Switch to the gpio descriptor interface

[Johan Hovold]

On Fri, Jan 11, 2019 at 09:03:16PM +0530, Nishad Kamdar wrote:
> This patch series converts uses of the old GPIO API to the GPIO
> descriptor API. It also converts the GPIO driver to use the
> GPIO irqchip library GPIOLIB_IRQCHIP instead of repimplementing
> the same.
> 
> Changes in v6:
>  - Patchset now contains two patches as the patch
>1 has been accepted.

All three patches look good now, but since Greg picks these up directly
and applies them to the staging tree, it is best if you submit all three
patches (with my reviewed-by tag) in a series.

One last resend as v7?

Thanks again for doing this.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 3/3] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold]

On Thu, Jan 10, 2019 at 11:23:07PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface while continuing to ignore gpio flags from device tree in
> "svc_reset_onoff()" for now.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v5:
>   - Change the commit message.
>   - Restore the names of the gpio device-tree properties without
> the "-gpio" suffix.
> Changes in v4:
>  - Move 'gpio_desc *svc_sysboot' below the reset flag
>as it is more logical to have reset flag below
>reset gpio.
>  - Remove a few unnecessary line breaks.
> Changes in v3:
>  - Add this patch to a patchset.
> Changes in v2:
>  - Move comment to the same line as to what it applies to.
> ---

Also looks good. Some really minor comments to a couple of the error
messages. The issues are there in the current code, but since you modify
these messages anyway you might as well fix them up. Please fix that and
resend with my

Reviewed-by: Johan Hovold 

Really good job with this!

> @@ -435,6 +428,7 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   struct device *dev = >dev;
>   struct device_node *np = dev->of_node;
>   int ret;
> + unsigned int flags;
>  
>   arche_pdata = devm_kzalloc(>dev, sizeof(*arche_pdata),
>  GFP_KERNEL);
> @@ -444,61 +438,33 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   /* setup svc reset gpio */
>   arche_pdata->is_reset_act_hi = of_property_read_bool(np,
>
> "svc,reset-active-high");
> - arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> - "svc,reset-gpio",
> - 0);
> - if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> - dev_err(dev, "failed to get reset-gpio\n");
> - return arche_pdata->svc_reset_gpio;
> - }
> - ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> - if (ret) {
> - dev_err(dev, "failed to request svc-reset gpio:%d\n", ret);
> - return ret;
> - }
> - ret = gpio_direction_output(arche_pdata->svc_reset_gpio,
> - arche_pdata->is_reset_act_hi);
> - if (ret) {
> - dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
> + if (arche_pdata->is_reset_act_hi)
> + flags = GPIOD_OUT_HIGH;
> + else
> + flags = GPIOD_OUT_LOW;
> +
> + arche_pdata->svc_reset = devm_gpiod_get(dev, "svc,reset", flags);
> + if (IS_ERR(arche_pdata->svc_reset)) {
> + ret = PTR_ERR(arche_pdata->svc_reset);
> + dev_err(dev, "failed to request svc-reset GPIO:%d\n", ret);

Add a space after ':' for consistency.

> @@ -515,19 +481,11 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   arche_pdata->num_apbs = of_get_child_count(np);
>   dev_dbg(dev, "Number of APB's available - %d\n", arche_pdata->num_apbs);
>  
> - arche_pdata->wake_detect_gpio = of_get_named_gpio(np,
> -   
> "svc,wake-detect-gpio",
> -   0);
> - if (arche_pdata->wake_detect_gpio < 0) {
> - dev_err(dev, "failed to get wake detect gpio\n");
> - return arche_pdata->wake_detect_gpio;
> - }
> -
> - ret = devm_gpio_request(dev, arche_pdata->wake_detect_gpio,
> - "wake detect");
> - if (ret) {
> - dev_err(dev, "Failed requesting wake_detect gpio %d\n",
> - arche_pdata->wake_detect_gpio);
> + arche_pdata->wake_detect = devm_gpiod_get(dev, "svc,wake-detect",
> +   GPIOD_IN);
> + if (IS_ERR(arche_pdata->wake_detect)) {
> + ret = PTR_ERR(arche_pdata->wake_detect);
> + dev_err(dev, "Failed requesting wake_detect GPIO %d\n", ret);

Add colon after "GPIO" for consistency (and to avoid ambiguity).

>   return ret;
>   }

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Johan Hovold]

On Thu, Jan 10, 2019 at 11:21:04PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface while continuing to ignore gpio flags from device tree in
> functions "deassert_reset()" and "assert_reset()" for now.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v5:
>  - Change the commit message.
>  - Restore the names of the gpio device-tree properties without
>the "-gpio" suffix.
> Changes in v4:
>  - Use gpiod_set_raw_value() for deassert_reset() and
>assert_reset() as gpiod_set_value() will change the
>sematics of these calls by taking any gpio flags
>into account.
>  - Remove some unnecesssary line breaks.
>  - Restore 'spi_en' gpio check in fw_flashing_seq()
>as it is currently optional.
> Changes in v3:
>  - Add this patch in a patchset.
> Changes in v2:
>  - Resolved compilation errors.
> ---

Also looks good now. You can add my

Reviewed-by: Johan Hovold 

Found one really minor nit below, which doesn't really need to be fixed,
but since you may need to update the third patch, you might as well
consider this too.

>   /* It's not mandatory to support power management interface */
> - apb->pwroff_gpio = of_get_named_gpio(np, "pwr-off-gpios", 0);
> - if (apb->pwroff_gpio < 0) {
> - dev_err(dev, "failed to get power off gpio\n");
> - return apb->pwroff_gpio;
> - }
> - ret = devm_gpio_request_one(dev, apb->pwroff_gpio,
> - GPIOF_IN, "pwroff_n");
> - if (ret) {
> - dev_err(dev, "Failed requesting pwroff_n gpio %d\n",
> - apb->pwroff_gpio);
> + apb->pwroff = devm_gpiod_get_optional(dev, "pwr-off",
> +   GPIOD_IN);

Looks like you don't need to break the above statement any more either.

> + if (IS_ERR(apb->pwroff)) {
> + ret = PTR_ERR(apb->pwroff);
> + dev_err(dev, "Failed requesting pwroff_n GPIO: %d\n", ret);
>   return ret;
>   }
>  
>   /* Do not make clock mandatory as of now (for DB3) */
> - apb->clk_en_gpio = of_get_named_gpio(np, "clock-en-gpio", 0);
> - if (apb->clk_en_gpio < 0) {
> - dev_warn(dev, "failed to get clock en gpio\n");
> - } else if (gpio_is_valid(apb->clk_en_gpio)) {
> - ret = devm_gpio_request_one(dev, apb->clk_en_gpio,
> - GPIOF_OUT_INIT_LOW, "apb_clk_en");
> - if (ret) {
> - dev_warn(dev, "Failed requesting APB clock en gpio 
> %d\n",
> -  apb->clk_en_gpio);
> - return ret;
> - }
> + apb->clk_en = devm_gpiod_get_optional(dev, "clock-en",
> +   GPIOD_OUT_LOW);

Same here?

> + if (IS_ERR(apb->clk_en)) {
> + ret = PTR_ERR(apb->clk_en);
> + dev_err(dev, "Failed requesting APB clock en GPIO: %d\n", ret);
> + return ret;
>   }

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/3] staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Johan Hovold]

On Thu, Jan 10, 2019 at 11:24:28PM +0530, Nishad Kamdar wrote:
> Convert the GPIO driver to use the GPIO irqchip library
> GPIOLIB_IRQCHIP instead of reimplementing the same.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v5:
>  - Restore "struct irq_chip irqc" in "struct gb_gpio_controller"
>This is because we cannot use the gpio-chip irqchip, as that
>will register the irqchip automatically and possibly in an
>incompatible way.
> Changes in v4:
>  - Remove changes related to conversion to gpiochip_get_data() to
>include it as a new patch.
>  - Remove the 'struct irq_chip' field from 'struct gb_gpio_controller'
>as struct gpio_chip will have an irqchip whenever
>CONFIG_GPIOLIB_IRQCHIP is selected.
>  - Update the TODO file as per the changes.
> Changes in v3:
>  - Combine patches as into a patch series.
> Changes in v2:
>  - Retained irq.h and irqdomain.h headers.
>  - Dropped function gb_gpio_irqchip_add() and
>called gpiochip_irqchip_add() from probe().
>  - Referred 
> https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.wall...@linaro.org.

Looks good to me now, thanks for sticking with it.

Reviewed-by: Johan Hovold 

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 4/4] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold]

On Sat, Dec 22, 2018 at 08:23:02PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated
> old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v4:
>  - Move 'gpio_desc *svc_sysboot' below the reset flag
>as it is more logical to have reset flag below
>reset gpio.
>  - Remove a few unnecessary line breaks.
> Changes in v3:
>  - Add this patch to a patchset.
> Changes in v2:
>  - Move comment to the same line as to what it applies to.
> ---

> -static inline void svc_reset_onoff(unsigned int gpio, bool onoff)
> +static inline void svc_reset_onoff(struct gpio_desc *gpio, bool onoff)
>  {
> - gpio_set_value(gpio, onoff);
> + gpiod_set_value(gpio, onoff);
>  }

Please use the raw interface here too, until we've done away with the
polarity properties and can honour the generic device tree flags. Please
make a comment about this in the commit message too.

> @@ -444,61 +438,33 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   /* setup svc reset gpio */
>   arche_pdata->is_reset_act_hi = of_property_read_bool(np,
>
> "svc,reset-active-high");
> - arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> - "svc,reset-gpio",
> - 0);
> - if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> - dev_err(dev, "failed to get reset-gpio\n");
> - return arche_pdata->svc_reset_gpio;
> - }
> - ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> - if (ret) {
> - dev_err(dev, "failed to request svc-reset gpio:%d\n", ret);
> - return ret;
> - }
> - ret = gpio_direction_output(arche_pdata->svc_reset_gpio,
> - arche_pdata->is_reset_act_hi);
> - if (ret) {
> - dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
> + if (arche_pdata->is_reset_act_hi)
> + flags = GPIOD_OUT_HIGH;
> + else
> + flags = GPIOD_OUT_LOW;
> +
> + arche_pdata->svc_reset = devm_gpiod_get(dev, "svc,reset-gpio", flags);

Again, you cannot just rename devicetree properties like this. Keep the
current names for now (and drop the -gpio suffix when requesting).

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/4] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Johan Hovold]

On Sat, Dec 22, 2018 at 08:22:09PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v4:
>  - Use gpiod_set_raw_value() for deassert_reset() and
>assert_reset() as gpiod_set_value() will change the
>sematics of these calls by taking any gpio flags
>into account.

Please also mention this in the commit message (i.e. that we continue to
ignore gpio flags from device tree for now).

>  - Remove some unnecesssary line breaks.
>  - Restore 'spi_en' gpio check in fw_flashing_seq()
>as it is currently optional.
> Changes in v3:
>  - Add this patch in a patchset.
> Changes in v2:
>  - Resolved compilation errors.
> ---

> @@ -75,11 +74,10 @@ static int coldboot_seq(struct platform_device *pdev)
>   return 0;
>  
>   /* Hold APB in reset state */
> - assert_reset(apb->resetn_gpio);
> + assert_reset(apb->resetn);
>  
> - if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> - gpio_is_valid(apb->spi_en_gpio))
> - devm_gpio_free(dev, apb->spi_en_gpio);
> + if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING && apb->spi_en)
> + devm_gpiod_put(dev, apb->spi_en);
>  
>   /* Enable power to APB */
>   if (!IS_ERR(apb->vcore)) {
> @@ -101,13 +99,13 @@ static int coldboot_seq(struct platform_device *pdev)
>   apb_bootret_deassert(dev);
>  
>   /* On DB3 clock was not mandatory */
> - if (gpio_is_valid(apb->clk_en_gpio))
> - gpio_set_value(apb->clk_en_gpio, 1);
> + if (apb->clk_en)
> + gpiod_set_value(apb->clk_en, 1);
>  
>   usleep_range(100, 200);
>  
>   /* deassert reset to APB : Active-low signal */
> - deassert_reset(apb->resetn_gpio);
> + deassert_reset(apb->resetn);
>  
>   apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
>  
> @@ -136,25 +134,25 @@ static int fw_flashing_seq(struct platform_device *pdev)
>   return ret;
>   }
>  
> - if (gpio_is_valid(apb->spi_en_gpio)) {
> + if (apb->spi_en) {
>   unsigned long flags;
>  
>   if (apb->spi_en_polarity_high)
> - flags = GPIOF_OUT_INIT_HIGH;
> + flags = GPIOD_OUT_HIGH;
>   else
> - flags = GPIOF_OUT_INIT_LOW;
> + flags = GPIOD_OUT_LOW;
>  
> - ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
> - flags, "apb_spi_en");
> - if (ret) {
> - dev_err(dev, "Failed requesting SPI bus en gpio %d\n",
> - apb->spi_en_gpio);
> + apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);

I just noticed that you change the name of the device-tree property here
(and later in apb_ctrl_get_devtree_data()). How is that expected to
work without breaking current systems? This will be unavoidable at some
point, but must not be snuck into a patch like this without any comment.
Please keep the current names for now.

I do think you need to drop the "-gpio" suffix when requesting the gpio
though. Please double check to make sure.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/4] staging: greybus: gpio: Use gpiochip_get_data() in place of gpio_chip_to_gb_gpio_controller()

[Johan Hovold]

On Sat, Dec 22, 2018 at 08:21:00PM +0530, Nishad Kamdar wrote:
> This patch drops gpio_chip_to_gb_gpio_controller(),
> and uses gpiochip_get_data() to retrieve the container
> of struct gpio_chip.

So this will break the driver, since gpiochip_add() sets the data
pointer to NULL.

These kind of changes are getting too complicated to do without
something to test against. We had a module simulator at one point, but
not sure what the state of that is now.

I appreciate the effort though, and since we already started, let's try
to finish, but please drop this patch for now.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/4] staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Johan Hovold]

On Sat, Dec 22, 2018 at 08:17:02PM +0530, Nishad Kamdar wrote:
> Convert the GPIO driver to use the GPIO irqchip library
> GPIOLIB_IRQCHIP instead of reimplementing the same.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v4:
>  - Remove changes related to conversion to gpiochip_get_data() to
>include it as a new patch.
>  - Remove the 'struct irq_chip' field from 'struct gb_gpio_controller'
>as struct gpio_chip will have an irqchip whenever
>CONFIG_GPIOLIB_IRQCHIP is selected.

Ok, sorry for misleading you this. It seems we cannot use the gpio-chip
irqchip, since that will register the irqchip automatically and possibly
in an incompatible way. This new functionality is far from well
documented, and you basically have to review the gpiolib code to figure
it out.

Looks like you need to add back the struct irq_chip.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/3] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold]

On Thu, Nov 22, 2018 at 10:39:24PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated
> old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v2:
>  - Move comment to the same line as to what it applies to.
> ---
>  drivers/staging/greybus/arche-platform.c | 119 ---
>  1 file changed, 41 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c 
> b/drivers/staging/greybus/arche-platform.c
> index 4c36e88766e7..a5cea79d8e32 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -8,10 +8,9 @@
>  
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -45,14 +44,14 @@ enum svc_wakedetect_state {
>  
>  struct arche_platform_drvdata {
>   /* Control GPIO signals to and from AP <=> SVC */
> - int svc_reset_gpio;
> + struct gpio_desc *svc_reset;
> + struct gpio_desc *svc_sysboot;

Why move sysboot? The flag below is about reset (but should eventually
go away).

>   bool is_reset_act_hi;
> - int svc_sysboot_gpio;
> - int wake_detect_gpio; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
> + struct gpio_desc *wake_detect; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME 
> signals */
>  
>   enum arche_platform_state state;
>  
> - int svc_refclk_req;
> + struct gpio_desc *svc_refclk_req;
>   struct clk *svc_ref_clk;
>  
>   struct pinctrl *pinctrl;
> @@ -85,9 +84,9 @@ static void arche_platform_set_wake_detect_state(
>   arche_pdata->wake_detect_state = state;
>  }
>  
> -static inline void svc_reset_onoff(unsigned int gpio, bool onoff)
> +static inline void svc_reset_onoff(struct gpio_desc *gpio, bool onoff)
>  {
> - gpio_set_value(gpio, onoff);
> + gpiod_set_value(gpio, onoff);
>  }
>  
>  static int apb_cold_boot(struct device *dev, void *data)
> @@ -116,7 +115,6 @@ static int apb_poweroff(struct device *dev, void *data)
>  static void arche_platform_wd_irq_en(struct arche_platform_drvdata 
> *arche_pdata)
>  {
>   /* Enable interrupt here, to read event back from SVC */
> - gpio_direction_input(arche_pdata->wake_detect_gpio);
>   enable_irq(arche_pdata->wake_detect_irq);
>  }
>  
> @@ -160,7 +158,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void 
> *devid)
>  
>   spin_lock_irqsave(_pdata->wake_lock, flags);
>  
> - if (gpio_get_value(arche_pdata->wake_detect_gpio)) {
> + if (gpiod_get_value(arche_pdata->wake_detect)) {
>   /* wake/detect rising */
>  
>   /*
> @@ -224,10 +222,10 @@ arche_platform_coldboot_seq(struct 
> arche_platform_drvdata *arche_pdata)
>  
>   dev_info(arche_pdata->dev, "Booting from cold boot state\n");
>  
> - svc_reset_onoff(arche_pdata->svc_reset_gpio,
> + svc_reset_onoff(arche_pdata->svc_reset,

No need to break line here.

>   arche_pdata->is_reset_act_hi);
>  
> - gpio_set_value(arche_pdata->svc_sysboot_gpio, 0);
> + gpiod_set_value(arche_pdata->svc_sysboot, 0);
>   usleep_range(100, 200);
>  
>   ret = clk_prepare_enable(arche_pdata->svc_ref_clk);
> @@ -238,7 +236,7 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata 
> *arche_pdata)
>   }
>  
>   /* bring SVC out of reset */
> - svc_reset_onoff(arche_pdata->svc_reset_gpio,
> + svc_reset_onoff(arche_pdata->svc_reset,

Same here. Please check throughout.

>   !arche_pdata->is_reset_act_hi);
>  
>   arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_ACTIVE);
> @@ -259,10 +257,10 @@ arche_platform_fw_flashing_seq(struct 
> arche_platform_drvdata *arche_pdata)
>  
>   dev_info(arche_pdata->dev, "Switching to FW flashing state\n");
>  
> - svc_reset_onoff(arche_pdata->svc_reset_gpio,
> + svc_reset_onoff(arche_pdata->svc_reset,
>   arche_pdata->is_reset_act_hi);
>  
> - gpio_set_value(arche_pdata->svc_sysboot_gpio, 1);
> + gpiod_set_value(arche_pdata->svc_sysboot, 1);
>  
>   usleep_range(100, 200);
>  
> @@ -273,7 +271,7 @@ arche_platform_fw_flashing_seq(struct 
> arche_platform_drvdata *arche_pdata)
>   return ret;
>   }
>  
> - svc_reset_onoff(arche_pdata->svc_reset_gpio,
> + svc_reset_onoff(arche_pdata->svc_reset,
>   !arche_pdata->is_reset_act_hi);
>  
>   arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_FW_FLASHING);
> @@ -305,7 +303,7 @@ arche_platform_poweroff_seq(struct arche_platform_drvdata 
> *arche_pdata)
>   clk_disable_unprepare(arche_pdata->svc_ref_clk);
>  
>   /* As part of exit, put APB back in reset state */
> - svc_reset_onoff(arche_pdata->svc_reset_gpio,
> + svc_reset_onoff(arche_pdata->svc_reset,
>   arche_pdata->is_reset_act_hi);
>  
>   arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_OFF);
> @@ -435,6 

Re: [PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Johan Hovold]

On Thu, Nov 22, 2018 at 10:38:18PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v2:
>  - Resolved compilation errors.
> ---
>  drivers/staging/greybus/arche-apb-ctrl.c | 159 +--
>  1 file changed, 65 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c 
> b/drivers/staging/greybus/arche-apb-ctrl.c
> index be5ffed90bcf..e887f6aee20b 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -8,9 +8,8 @@
>  
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -24,12 +23,12 @@ static void apb_bootret_deassert(struct device *dev);
>  
>  struct arche_apb_ctrl_drvdata {
>   /* Control GPIO signals to and from AP <=> AP Bridges */
> - int resetn_gpio;
> - int boot_ret_gpio;
> - int pwroff_gpio;
> - int wake_in_gpio;
> - int wake_out_gpio;
> - int pwrdn_gpio;
> + struct gpio_desc *resetn;
> + struct gpio_desc *boot_ret;
> + struct gpio_desc *pwroff;
> + struct gpio_desc *wake_in;
> + struct gpio_desc *wake_out;
> + struct gpio_desc *pwrdn;
>  
>   enum arche_platform_state state;
>   bool init_disabled;
> @@ -37,28 +36,28 @@ struct arche_apb_ctrl_drvdata {
>   struct regulator *vcore;
>   struct regulator *vio;
>  
> - int clk_en_gpio;
> + struct gpio_desc *clk_en;
>   struct clk *clk;
>  
>   struct pinctrl *pinctrl;
>   struct pinctrl_state *pin_default;
>  
>   /* V2: SPI Bus control  */
> - int spi_en_gpio;
> + struct gpio_desc *spi_en;
>   bool spi_en_polarity_high;
>  };
>  
>  /*
>   * Note that these low level api's are active high
>   */
> -static inline void deassert_reset(unsigned int gpio)
> +static inline void deassert_reset(struct gpio_desc *gpio)
>  {
> - gpio_set_value(gpio, 1);
> + gpiod_set_value(gpio, 1);
>  }
>  
> -static inline void assert_reset(unsigned int gpio)
> +static inline void assert_reset(struct gpio_desc *gpio)
>  {
> - gpio_set_value(gpio, 0);
> + gpiod_set_value(gpio, 0);
>  }

As the comment above deassert_reset() suggests, this change will
actually change the semantics of these calls by taking any gpio flags
into account (e.g. ACTIVE_LOW which will invert the signals).

Perhaps you should just use gpiod_set_raw_value() for now, and this can
be addressed later. Alternatively, drop the comment and invert the
polarity here and any users will need to update their device trees.

Either way, mention this in the commit message.

>  /*
> @@ -75,11 +74,11 @@ static int coldboot_seq(struct platform_device *pdev)
>   return 0;
>  
>   /* Hold APB in reset state */
> - assert_reset(apb->resetn_gpio);
> + assert_reset(apb->resetn);
>  
>   if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> - gpio_is_valid(apb->spi_en_gpio))
> - devm_gpio_free(dev, apb->spi_en_gpio);
> + apb->spi_en)

No need to break the line any more.

> + devm_gpiod_put(dev, apb->spi_en);
>  
>   /* Enable power to APB */
>   if (!IS_ERR(apb->vcore)) {
> @@ -101,13 +100,13 @@ static int coldboot_seq(struct platform_device *pdev)
>   apb_bootret_deassert(dev);
>  
>   /* On DB3 clock was not mandatory */
> - if (gpio_is_valid(apb->clk_en_gpio))
> - gpio_set_value(apb->clk_en_gpio, 1);
> + if (apb->clk_en)
> + gpiod_set_value(apb->clk_en, 1);
>  
>   usleep_range(100, 200);
>  
>   /* deassert reset to APB : Active-low signal */
> - deassert_reset(apb->resetn_gpio);
> + deassert_reset(apb->resetn);
>  
>   apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
>  
> @@ -119,6 +118,7 @@ static int fw_flashing_seq(struct platform_device *pdev)
>   struct device *dev = >dev;
>   struct arche_apb_ctrl_drvdata *apb = platform_get_drvdata(pdev);
>   int ret;
> + unsigned long flags;
>  
>   if (apb->init_disabled ||
>   apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING)
> @@ -136,25 +136,20 @@ static int fw_flashing_seq(struct platform_device *pdev)
>   return ret;
>   }
>  
> - if (gpio_is_valid(apb->spi_en_gpio)) {

spi_en_gpio is currently optional, so cannot just drop the check.

> - unsigned long flags;
> + if (apb->spi_en_polarity_high)
> + flags = GPIOD_OUT_HIGH;
> + else
> + flags = GPIOD_OUT_LOW;

This should probably also be converted to honouring the gpio flags, but
perhaps better to do in a later patch.

>  
> - if (apb->spi_en_polarity_high)
> - flags = GPIOF_OUT_INIT_HIGH;
> - else
> - flags = GPIOF_OUT_INIT_LOW;
> -
> - ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
> - 

Re: [PATCH v3 1/3] staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Johan Hovold]

On Thu, Nov 22, 2018 at 10:37:16PM +0530, Nishad Kamdar wrote:
> Convert the GPIO driver to use the GPIO irqchip library
> GPIOLIB_IRQCHIP instead of reimplementing the same.
> 
> Signed-off-by: Nishad Kamdar 
> ---
> Changes in v2:
>  - Retained irq.h and irqdomain.h headers.
>  - Dropped function gb_gpio_irqchip_add() and
>called gpiochip_irqchip_add() from probe().
>  - Referred 
> https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.wall...@linaro.org.

Thanks for the update, and sorry about the late review. This looks
mostly good now, except for a couple minor things pointed out below.

You also included the conversion to gpiochip_get_data() (as Linus also
did in his patch) although that's really a separate change and should go
in its own patch. Please break that bit out in a follow-up patch.

Also note that someone did a bunch random white space changes to this
file in the staging tree, so it will not apply cleanly any more.

> ---
>  drivers/staging/greybus/Kconfig |   1 +
>  drivers/staging/greybus/gpio.c  | 184 
>  2 files changed, 24 insertions(+), 161 deletions(-)
> 
> diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
> index ab096bcef98c..b571e4e8060b 100644
> --- a/drivers/staging/greybus/Kconfig
> +++ b/drivers/staging/greybus/Kconfig
> @@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
>  config GREYBUS_GPIO
>   tristate "Greybus GPIO Bridged PHY driver"
>   depends on GPIOLIB
> + select GPIOLIB_IRQCHIP
>   ---help---
> Select this option if you have a device that follows the
> Greybus GPIO Bridged PHY Class specification.
> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> index b1d4698019a1..2ec54744171d 100644
> --- a/drivers/staging/greybus/gpio.c
> +++ b/drivers/staging/greybus/gpio.c
> @@ -9,9 +9,9 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "greybus.h"
> @@ -39,15 +39,8 @@ struct gb_gpio_controller {
>  
>   struct gpio_chipchip;
>   struct irq_chip irqc;

Turns out struct gpio_chip will have an irqchip whenever
CONFIG_GPIOLIB_IRQCHIP is selected so you can drop this one too.

> - struct irq_chip *irqchip;
> - struct irq_domain   *irqdomain;
> - unsigned intirq_base;
> - irq_flow_handler_t  irq_handler;
> - unsigned intirq_default_type;
>   struct mutexirq_lock;
>  };
> -#define gpio_chip_to_gb_gpio_controller(chip) \
> - container_of(chip, struct gb_gpio_controller, chip)
>  #define irq_data_to_gpio_chip(d) (d->domain->host_data)
>  
>  static int gb_gpio_line_count_operation(struct gb_gpio_controller *ggc)
> @@ -276,7 +269,7 @@ static void _gb_gpio_irq_set_type(struct 
> gb_gpio_controller *ggc,
>  static void gb_gpio_irq_mask(struct irq_data *d)
>  {
>   struct gpio_chip *chip = irq_data_to_gpio_chip(d);
> - struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
> + struct gb_gpio_controller *ggc = gpiochip_get_data(chip);

So please split these changes into a separate patch as they are not
related to the irqchip changes.

Oh, and don't forget to update the TODO file now that the conversion is
done. :)

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold]

On Sat, Nov 17, 2018 at 12:41:11PM +0530, Nishad Kamdar wrote:
> On Fri, Nov 16, 2018 at 05:06:22PM +0100, Johan Hovold wrote:
> > On Fri, Nov 16, 2018 at 08:47:44PM +0530, Nishad Kamdar wrote:
> > > Use the gpiod interface instead of the deprecated
> > > old non-descriptor interface.
> > > 
> > > Signed-off-by: Nishad Kamdar 
> > > ---
> > 
> > Always include a change log here after the cut-off line so we know what
> > changed since previous versions.
> > 
> > Also include the patch revision in the Subject (e.g. "[PATCH v3]
> > staging: greybus: ...").
> > 
> 
> Ok, but this is the first patch version that I submitted
> for greybus: arche-platform.

Ah, sorry. I thought this was an update of the arche-apb-ctrl patch.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold]

On Fri, Nov 16, 2018 at 08:47:44PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated
> old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 
> ---

Always include a change log here after the cut-off line so we know what
changed since previous versions.

Also include the patch revision in the Subject (e.g. "[PATCH v3]
staging: greybus: ...").

>  drivers/staging/greybus/arche-platform.c | 120 ---
>  1 file changed, 42 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c 
> b/drivers/staging/greybus/arche-platform.c
> index 4c36e88766e7..a826a1835628 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -8,10 +8,9 @@
>  
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -45,14 +44,15 @@ enum svc_wakedetect_state {
>  
>  struct arche_platform_drvdata {
>   /* Control GPIO signals to and from AP <=> SVC */
> - int svc_reset_gpio;
> + struct gpio_desc *svc_reset;
> + struct gpio_desc *svc_sysboot;
>   bool is_reset_act_hi;
> - int svc_sysboot_gpio;
> - int wake_detect_gpio; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
> + struct gpio_desc *wake_detect;
> + /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */

I'm not commenting on the rest, but comments never go underneath what
they apply to.

Just keep the current comment here, even if it's placement is a bit odd
and makes the line be longer than 80 cols.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Johan Hovold]

On Tue, Nov 13, 2018 at 11:48:09PM +0530, Nishad Kamdar wrote:
> On Mon, Nov 12, 2018 at 04:15:09PM +0100, Johan Hovold wrote:
> > On Fri, Nov 09, 2018 at 01:17:41PM +0530, Nishad Kamdar wrote:
> > > @@ -40,8 +38,6 @@ struct gb_gpio_controller {

> > >   struct gpio_chipchip;
> > >   struct irq_chip irqc;
> > >   struct irq_chip *irqchip;
> > 
> > This should not be needed any more either.
> 
> Just to confirm, by thism you mean only 
> struct irq_chip   *irqchip; right?

Right.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Johan Hovold]

On Sun, Nov 11, 2018 at 11:14:14AM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
> 
> Signed-off-by: Nishad Kamdar 
> ---
>  drivers/staging/greybus/arche-apb-ctrl.c | 158 ++-
>  1 file changed, 65 insertions(+), 93 deletions(-)

> - apb->pwrdn_gpio = of_get_named_gpio(np, "pwr-down-gpios", 0);
> - if (apb->pwrdn_gpio < 0)
> - dev_warn(dev, "failed to get power down gpio\n");
> + apb->pwrdn = devm_gpiod_get(dev, "gb,pwr-down-gpios", GPIOD_OUT_LOW);
> + if (IF_ERR(apb->pwrdn)) {

Looks like you didn't even compile test this one. :(

> + ret = PTR_ERR(apb->pwrdn);
> + dev_warn(dev, "Failed requesting power down GPIO: %d\n", ret);
> + return ret;
> + }

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Johan Hovold]

On Fri, Nov 09, 2018 at 01:17:41PM +0530, Nishad Kamdar wrote:
> Convert the GPIO driver to use the GPIO irqchip library
> GPIOLIB_IRQCHIP instead of reimplementing the same.

Thanks for picking this up. Linus W took a stab at it a couple of years
ago, but it was never completed. You may want to take a look at that
thread:


https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.wall...@linaro.org

> Signed-off-by: Nishad Kamdar 
> ---
>  drivers/staging/greybus/Kconfig |   1 +
>  drivers/staging/greybus/gpio.c  | 123 ++--
>  2 files changed, 21 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
> index ab096bcef98c..b571e4e8060b 100644
> --- a/drivers/staging/greybus/Kconfig
> +++ b/drivers/staging/greybus/Kconfig
> @@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
>  config GREYBUS_GPIO
>   tristate "Greybus GPIO Bridged PHY driver"
>   depends on GPIOLIB
> + select GPIOLIB_IRQCHIP
>   ---help---
> Select this option if you have a device that follows the
> Greybus GPIO Bridged PHY Class specification.
> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> index b1d4698019a1..32c228bad33a 100644
> --- a/drivers/staging/greybus/gpio.c
> +++ b/drivers/staging/greybus/gpio.c
> @@ -9,9 +9,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> -#include 

I think you should keep irq.h even if the gpio header currently provides
it.

> -#include 

Similarly for this one, if you still rely on it.

> +#include 
>  #include 
>  
>  #include "greybus.h"
> @@ -40,8 +38,6 @@ struct gb_gpio_controller {
>   struct gpio_chipchip;
>   struct irq_chip irqc;
>   struct irq_chip *irqchip;

This should not be needed any more either.

> - struct irq_domain   *irqdomain;
> - unsigned intirq_base;
>   irq_flow_handler_t  irq_handler;
>   unsigned intirq_default_type;

Neither should these two.

>   struct mutexirq_lock;
> @@ -365,6 +361,7 @@ static int gb_gpio_request_handler(struct gb_operation 
> *op)
>  {
>   struct gb_connection *connection = op->connection;
>   struct gb_gpio_controller *ggc = gb_connection_get_data(connection);
> + struct gpio_chip *gc = >chip;

Please name the pointer 'chip' as in the rest of the driver to avoid
confusion with 'ggc'. But looks like you don't need it at all.

>   struct device *dev = >gbphy_dev->dev;
>   struct gb_message *request;
>   struct gb_gpio_irq_event_request *event;
> @@ -391,7 +388,7 @@ static int gb_gpio_request_handler(struct gb_operation 
> *op)
>   return -EINVAL;
>   }
>  
> - irq = irq_find_mapping(ggc->irqdomain, event->which);
> + irq = irq_find_mapping(gc->irq.domain, event->which);
>   if (!irq) {
>   dev_err(dev, "failed to find IRQ\n");
>   return -EINVAL;
> @@ -506,68 +503,6 @@ static int gb_gpio_controller_setup(struct 
> gb_gpio_controller *ggc)
>   return ret;
>  }

> -/**
> - * gb_gpio_irqchip_remove() - removes an irqchip added to a 
> gb_gpio_controller
> - * @ggc: the gb_gpio_controller to remove the irqchip from
> - *
> - * This is called only from gb_gpio_remove()
> - */
> -static void gb_gpio_irqchip_remove(struct gb_gpio_controller *ggc)
> -{
> - unsigned int offset;
> -
> - /* Remove all IRQ mappings and delete the domain */
> - if (ggc->irqdomain) {
> - for (offset = 0; offset < (ggc->line_max + 1); offset++)
> - irq_dispose_mapping(irq_find_mapping(ggc->irqdomain,
> -  offset));
> - irq_domain_remove(ggc->irqdomain);
> - }
> -
> - if (ggc->irqchip)
> - ggc->irqchip = NULL;
> -}
> -
>  /**
>   * gb_gpio_irqchip_add() - adds an irqchip to a gpio chip
>   * @chip: the gpio chip to add the irqchip to
> @@ -595,8 +530,7 @@ static int gb_gpio_irqchip_add(struct gpio_chip *chip,
>unsigned int type)
>  {
>   struct gb_gpio_controller *ggc;
> - unsigned int offset;
> - unsigned int irq_base;
> + unsigned int err;
>  
>   if (!chip || !irqchip)
>   return -EINVAL;
> @@ -606,35 +540,21 @@ static int gb_gpio_irqchip_add(struct gpio_chip *chip,
>   ggc->irqchip = irqchip;
>   ggc->irq_handler = handler;
>   ggc->irq_default_type = type;
> - ggc->irqdomain = irq_domain_add_simple(NULL,
> - ggc->line_max + 1, first_irq,
> - _gpio_domain_ops, chip);
> - if (!ggc->irqdomain) {
> - ggc->irqchip = NULL;
> - return -EINVAL;
> - }
>  
> - /*
> -  * Prepare the mapping since the irqchip shall be orthogonal to
> -  * any gpio calls. If the first_irq was zero, this is
> -  * necessary to allocate descriptors for all 

Re: [PATCH] staging: greybus: Fix null pointer dereference

[Johan Hovold]

On Fri, Aug 24, 2018 at 12:07:11AM -0400, Ding Xiang wrote:
> If fw is null then fw->size will trigger null pointer dereference
>
> Signed-off-by: Ding Xiang 
> ---
>  drivers/staging/greybus/bootrom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/bootrom.c 
> b/drivers/staging/greybus/bootrom.c
> index e85ffae..3af28a0 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -297,7 +297,7 @@ static int gb_bootrom_get_firmware(struct gb_operation 
> *op)
>  
>  queue_work:
>   /* Refresh timeout */
> - if (!ret && (offset + size == fw->size))
> + if (!ret && fw && (offset + size == fw->size))
>   next_request = NEXT_REQ_READY_TO_BOOT;
>   else
>   next_request = NEXT_REQ_GET_FIRMWARE;

How could fw be NULL when ret is 0 here?

It may not be as obvious as one might have wished, but the current code
looks correct to me.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] MAINTAINERS: update two greybus sections

[Johan Hovold]

Fix a file entry typo and drop the obsolete timesync entries, which were
all caught by:

scripts/get_maintainer.pl --self-test=patterns

Reported-by: Joe Perches 
Signed-off-by: Johan Hovold 
---

This has been reported and at least partially fixed in the past, but due
to various other clean-up work that was going on in MAINTAINERS at the
time, was never applied.

Johan


 MAINTAINERS | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9c125f705f78..09d7f9eb5c13 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6065,7 +6065,7 @@ F:drivers/staging/greybus/bootrom.c
 F: drivers/staging/greybus/firmware.h
 F: drivers/staging/greybus/fw-core.c
 F: drivers/staging/greybus/fw-download.c
-F: drivers/staging/greybus/fw-managament.c
+F: drivers/staging/greybus/fw-management.c
 F: drivers/staging/greybus/greybus_authentication.h
 F: drivers/staging/greybus/greybus_firmware.h
 F: drivers/staging/greybus/hid.c
@@ -6074,12 +6074,10 @@ F:  drivers/staging/greybus/spi.c
 F: drivers/staging/greybus/spilib.c
 F: drivers/staging/greybus/spilib.h
 
-GREYBUS LOOPBACK/TIME PROTOCOLS DRIVERS
+GREYBUS LOOPBACK DRIVER
 M: Bryan O'Donoghue 
 S: Maintained
 F: drivers/staging/greybus/loopback.c
-F: drivers/staging/greybus/timesync.c
-F: drivers/staging/greybus/timesync_platform.c
 
 GREYBUS PLATFORM DRIVERS
 M: Vaibhav Hiremath 
-- 
2.17.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [greybus-dev] [PATCH] Staging:greybus Fix comparison to NULL

[Johan Hovold]

On Tue, Jun 05, 2018 at 11:02:36AM +0530, Viresh Kumar wrote:
> On 03-06-18, 08:52, Janani Sankara Babu wrote:
> > This patch replaces comparison of var to NULL with !var
> > 
> > Signed-off-by: Janani Sankara Babu 
> > ---
> >  drivers/staging/greybus/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c
> > index dafa430..5d14a4e 100644
> > --- a/drivers/staging/greybus/core.c
> > +++ b/drivers/staging/greybus/core.c
> > @@ -48,7 +48,7 @@ static bool greybus_match_one_id(struct gb_bundle *bundle,
> >  static const struct greybus_bundle_id *
> >  greybus_match_id(struct gb_bundle *bundle, const struct greybus_bundle_id 
> > *id)
> >  {
> > -   if (id == NULL)
> > +   if (!id)
> 
> It is pretty much personal preference and there is no good reason to
> accept this patch really.

I agree with you, Viresh, but greybus is still living in staging which
makes checkpatch enable the --strict option, thereby forcing its authors
preferences onto the rest of us.

I also do understand Greg's point that while --strict is enabled,
rejecting such patches will probably cause some new submitters to do
work for nothing, which is not good.

On the other hand, I at least think we should enforce having a patch
description that clearly indicates that this is just something to make
checkpatch happy (reserving the term "fix" for proper fixes, at least if
not used in conjunction with "checkpatch warning").

So Janani, would you mind resubmitting this patch with a slightly
modified commit message, such as:

staging: greybus: clean up NULL comparisons

Replace comparison of var to NULL with !var in order to address
a checkpatch (strict) warning.

You could also change the sole remaining comparison to NULL in bundle.c
in the same patch.

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/13] staging: greybus: camera: no need to check debugfs return values

[Johan Hovold]

On Tue, May 29, 2018 at 04:29:35PM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Clean up the greybus camera driver by not caring about the value of
> debugfs calls.  This ends up removing a number of lines of code that
> are not needed.
> 
> Cc: Johan Hovold 
> Cc: Alex Elder 
> Cc: Greg Kroah-Hartman 
> Cc: greybus-...@lists.linaro.org
> Signed-off-by: Greg Kroah-Hartman 

Acked-by: Johan Hovold 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [greybus-dev] [PATCH] staging: greybus: Remove unused local variable

[Johan Hovold]

On Mon, May 07, 2018 at 10:35:46AM +0530, Viresh Kumar wrote:
> On 05-05-18, 23:50, Nathan Chancellor wrote:
> > Fixes the following W=1 warning: variable ‘intf_id’ set but
> > not used [-Wunused-but-set-variable]
> > 
> > Signed-off-by: Nathan Chancellor <natechancel...@gmail.com>
> > ---
> >  drivers/staging/greybus/svc.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/svc.c b/drivers/staging/greybus/svc.c
> > index a874fed761a1..a2bb7e1a3db3 100644
> > --- a/drivers/staging/greybus/svc.c
> > +++ b/drivers/staging/greybus/svc.c
> > @@ -1137,7 +1137,6 @@ static int gb_svc_intf_reset_recv(struct gb_operation 
> > *op)
> > struct gb_svc *svc = gb_connection_get_data(op->connection);
> > struct gb_message *request = op->request;
> > struct gb_svc_intf_reset_request *reset;
> > -   u8 intf_id;
> >  
> > if (request->payload_size < sizeof(*reset)) {
> > dev_warn(>dev, "short reset request received (%zu < 
> > %zu)\n",
> > @@ -1146,8 +1145,6 @@ static int gb_svc_intf_reset_recv(struct gb_operation 
> > *op)
> > }
> > reset = request->payload;
> >  
> > -   intf_id = reset->intf_id;
> > -
> > /* FIXME Reset the interface here */
> >  
> > return 0;
> 
> Don't you get a new error after removing this, i.e "reset set but unused" ? Or
> the sizeof() operation on that suppresses those warnings ..

That was my initial reaction as well, but I failed to notice the sizeof
which prevents the new warning.

> Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

Acked-by: Johan Hovold <jo...@kernel.org>

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Use gpio_is_valid()

[Johan Hovold]

On Wed, May 02, 2018 at 03:15:05PM +0530, Arvind Yadav wrote:

> On Wednesday 02 May 2018 02:13 PM, Johan Hovold wrote:
> > On Sat, Apr 28, 2018 at 10:05:39AM +0530, Arvind Yadav wrote:
> >> Replace the manual validity checks for the GPIO with the
> >> gpio_is_valid().
> >>
> >> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
> >> ---
> >> chnage in v2 :
> >>   Returning invalid gpio as error instead of -ENODEV.
> >>
> >>   drivers/staging/greybus/arche-platform.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/staging/greybus/arche-platform.c 
> >> b/drivers/staging/greybus/arche-platform.c
> >> index 83254a7..c3a7da5 100644
> >> --- a/drivers/staging/greybus/arche-platform.c
> >> +++ b/drivers/staging/greybus/arche-platform.c
> >> @@ -448,7 +448,7 @@ static int arche_platform_probe(struct platform_device 
> >> *pdev)
> >>arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> >>"svc,reset-gpio",
> >>0);
> >> -  if (arche_pdata->svc_reset_gpio < 0) {
> >> +  if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> >>dev_err(dev, "failed to get reset-gpio\n");
> >>return arche_pdata->svc_reset_gpio;
> >
> > I'm sorry, but I don't this change is desirable. of_get_named_gpio()
> > returns a valid gpio number or a negative errno, so there's no need to
> > use the legacy gpio_is_valid() helper here.
> >
> > If you grep for of_get_named_gpio() you'll find that some drivers indeed
> > use that helper this way, but they are in a clear minority.
> >
> > And ultimately, we want to move to using gpio descriptors anyway.
> 
> We need to check gpio validity. If we are using of_get_named_gpio() or
> not.  of_get_name_gpio() will read a device node and fetch the value.
> But it'll not check that gpio is valid or not valid.

No, I believe you're mistaken here. of_get_named_gpio() does not return
an arbitrary gpio number, unlike what you could possibly find in
legacy board files and for which the gpio_is_valid() helper made sense.

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Use gpio_is_valid()

[Johan Hovold]

On Sat, Apr 28, 2018 at 10:05:39AM +0530, Arvind Yadav wrote:
> Replace the manual validity checks for the GPIO with the
> gpio_is_valid().
> 
> Signed-off-by: Arvind Yadav 
> ---
> chnage in v2 :
>  Returning invalid gpio as error instead of -ENODEV.
> 
>  drivers/staging/greybus/arche-platform.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c 
> b/drivers/staging/greybus/arche-platform.c
> index 83254a7..c3a7da5 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -448,7 +448,7 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
>   "svc,reset-gpio",
>   0);
> - if (arche_pdata->svc_reset_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
>   dev_err(dev, "failed to get reset-gpio\n");
>   return arche_pdata->svc_reset_gpio;

I'm sorry, but I don't this change is desirable. of_get_named_gpio()
returns a valid gpio number or a negative errno, so there's no need to
use the legacy gpio_is_valid() helper here.

If you grep for of_get_named_gpio() you'll find that some drivers indeed
use that helper this way, but they are in a clear minority.

And ultimately, we want to move to using gpio descriptors anyway.

>   }
> @@ -468,7 +468,7 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
> "svc,sysboot-gpio",
> 0);
> - if (arche_pdata->svc_sysboot_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
>   dev_err(dev, "failed to get sysboot gpio\n");
>   return arche_pdata->svc_sysboot_gpio;
>   }
> @@ -487,7 +487,7 @@ static int arche_platform_probe(struct platform_device 
> *pdev)
>   arche_pdata->svc_refclk_req = of_get_named_gpio(np,
>   "svc,refclk-req-gpio",
>   0);
> - if (arche_pdata->svc_refclk_req < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
>   dev_err(dev, "failed to get svc clock-req gpio\n");
>   return arche_pdata->svc_refclk_req;
>   }

But if this were to be changed, you'd need to update also the fourth
call to of_get_named_gpio() in this function.

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/9] staging: greybus: Add TODO file with GPIO work items

[Johan Hovold]

On Thu, Apr 19, 2018 at 10:41:15AM +0200, Linus Walleij wrote:
> To make sure that these drivers do not leave staging before they
> are properly converted to use the new GPIO descriptor API, and the
> GPIOLIB_IRQCHIP helper library, create the TODO file with these work
> items.
> 
> Cc: Johan Hovold <jo...@kernel.org>
> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>

Thanks for documenting this in the tree. This code had to run on old
Android kernels so we could not use fancy things like gpio descriptors
and the gpio-irqchip implementation at the time. ;)

> ---
> I started some work in this area, make sure to just throw me in
> on CC whenever anyone works on it and I will happily help and
> provide examples!
> ---
>  drivers/staging/greybus/TODO | 5 +
>  1 file changed, 5 insertions(+)
>  create mode 100644 drivers/staging/greybus/TODO
> 
> diff --git a/drivers/staging/greybus/TODO b/drivers/staging/greybus/TODO
> new file mode 100644
> index ..3b90a5711998
> --- /dev/null
> +++ b/drivers/staging/greybus/TODO
> @@ -0,0 +1,5 @@
> +* Convert all uses of the old GPIO API from  to the
> +  GPIO descriptor API in  and look up GPIO
> +  lines from device tree or ACPI.

Not sure that this sentence makes sense for greybus however. We're
querying the module about how many lines it provides using the greybus
protocol, and while we had some ideas about moving some such
descriptions to device-tree fragments (which we'd retrieve from the
module instead) that's probably not going to happen for a while.

The chance of ACPI being used for this is nil in any case.

> +* Convert the GPIO driver to use the GPIO irqchip library
> +  GPIOLIB_IRQCHIP instead of reimplementing the same.

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 47/61] staging: greybus: simplify getting .drvdata

[Johan Hovold]

On Thu, Apr 19, 2018 at 04:06:17PM +0200, Wolfram Sang wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Acked-by: Johan Hovold <jo...@kernel.org>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: irda: Replace mdelay with usleep_range in irda_usb_probe

[Johan Hovold]

On Wed, Apr 11, 2018 at 09:33:55AM +0800, Jia-Ju Bai wrote:
> irda_usb_probe() is never called in atomic context.
> 
> irda_usb_probe() is only set as ".probe" in struct usb_driver.
> 
> Despite never getting called from atomic context, irda_usb_probe()
> calls mdelay() to busily wait.
> This is not necessary and can be replaced with usleep_range() to
> avoid busy waiting.
> 
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1...@gmail.com>

Reviewed-by: Johan Hovold <jo...@kernel.org>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload

[Johan Hovold]

On Wed, Apr 11, 2018 at 09:29:34AM +0800, Jia-Ju Bai wrote:
> stir421x_fw_upload() is never called in atomic context.
> 
> The call chain ending up at stir421x_fw_upload() is:
> [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe()
> 
> irda_usb_probe() is set as ".probe" in struct usb_driver.
> This function is not called in atomic context.
> 
> Despite never getting called from atomic context, stir421x_fw_upload()
> calls mdelay() to busily wait.
> This is not necessary and can be replaced with usleep_range() to
> avoid busy waiting.
> 
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1...@gmail.com>

This all looks good (also note the call to usb_control_msg(), which may
sleep, just above the mdelay()).

Reviewed-by: Johan Hovold <jo...@kernel.org>

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] Staging: greybus: camera: cleanup multiple checks for null pointers

[Johan Hovold]

On Mon, Jan 08, 2018 at 10:20:15PM +0530, Sumit Pundir wrote:
> Fixed coding style issue regarding null comparison at multiple lines.
> Issue reported by checkpatch.pl
> 
> Signed-off-by: Sumit Pundir <pundirsumi...@gmail.com>
> ---
> v2:
>  Updated the patch title and description.

Thanks for the update.

Acked-by: Johan Hovold <jo...@kernel.org>

Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: greybus: Fix multiple checks for null pointers

[Johan Hovold]

On Mon, Jan 08, 2018 at 11:28:13AM +0530, Sumit Pundir wrote:
> Fixes the following coding style issue as noted by checkpatch.pl
> at multiple lines:
> 
> Comparison to NULL could be written "!token"
> 
> Signed-off-by: Sumit Pundir 

Since you're not really fixing anything here, besides silencing a
checkpatch suggestion when run with the --strict option (or on staging
code), I suggest you reword you commit summary (Subject) to, for
example:

staging: greybus: camera: clean up NULL checks

or similar.

Note that I also added "camera" as a module prefix above.

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: greybus: audio_codec.h: Change uint32_t to u32

[Johan Hovold]

On Wed, Jan 03, 2018 at 03:07:33PM +0530, Sumit Pundir wrote:
> This patch fixes the following checkpatch.pl issues:
> 
> CHECK: Prefer kernel type 'u32' over 'uint32_t'
> +   uint32_t format, rate;
> 
> CHECK: Prefer kernel type 'u32' over 'uint32_t'
> +  uint32_t *rate, u8 *channels,
> 
> CHECK: Prefer kernel type 'u32' over 'uint32_t'
> +  uint32_t rate, u8 channels,
> 
> Signed-off-by: Sumit Pundir 
> ---
>  drivers/staging/greybus/audio_codec.h | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

Please fold this in with the changes to audio_codec.c and submit a
single patch for it all.

Thanks,
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel