[PATCH] staging: fwserial: Fix error handling in fwserial_create

[Dinghao Liu]

When fw_core_add_address_handler() fails, we need to destroy
the port by tty_port_destroy(). Also we need to unregister
the address handler by fw_core_remove_address_handler() on
failure.

Signed-off-by: Dinghao Liu 
---
 drivers/staging/fwserial/fwserial.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/fwserial/fwserial.c 
b/drivers/staging/fwserial/fwserial.c
index db83d34cd677..c368082aae1a 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -2189,6 +2189,7 @@ static int fwserial_create(struct fw_unit *unit)
err = fw_core_add_address_handler(&port->rx_handler,
  &fw_high_memory_region);
if (err) {
+   tty_port_destroy(&port->port);
kfree(port);
goto free_ports;
}
@@ -2271,6 +2272,7 @@ static int fwserial_create(struct fw_unit *unit)
 
 free_ports:
for (--i; i >= 0; --i) {
+   fw_core_remove_address_handler(&serial->ports[i]->rx_handler);
tty_port_destroy(&serial->ports[i]->port);
kfree(serial->ports[i]);
}
-- 
2.17.1

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

[PATCH] staging: rtl8192u: Add null check in rtl8192_usb_initendpoints

[Dinghao Liu]

There is an allocation for priv->rx_urb[16] has no null check,
which may lead to a null pointer dereference.

Signed-off-by: Dinghao Liu 
---
 drivers/staging/rtl8192u/r8192U_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
b/drivers/staging/rtl8192u/r8192U_core.c
index 93676af98629..9fc4adc83d77 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -1608,6 +1608,8 @@ static short rtl8192_usb_initendpoints(struct net_device 
*dev)
void *oldaddr, *newaddr;
 
priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
+   if (!priv->rx_urb[16])
+   return -ENOMEM;
priv->oldaddr = kmalloc(16, GFP_KERNEL);
if (!priv->oldaddr)
return -ENOMEM;
-- 
2.17.1

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

[PATCH] media: staging: tegra-vde: fix runtime pm imbalance on error

[Dinghao Liu]

pm_runtime_get_sync() increments the runtime PM usage counter even
it returns an error code. Thus a pairing decrement is needed on
the error handling path to keep the counter balanced.

Signed-off-by: Dinghao Liu 
---
 drivers/staging/media/tegra-vde/vde.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/tegra-vde/vde.c 
b/drivers/staging/media/tegra-vde/vde.c
index d3e63512a765..dd134a3a15c7 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -777,7 +777,7 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde 
*vde,
 
ret = pm_runtime_get_sync(dev);
if (ret < 0)
-   goto unlock;
+   goto put_runtime_pm;
 
/*
 * We rely on the VDE registers reset value, otherwise VDE
-- 
2.17.1

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

Re: Re: [PATCH] media: staging: tegra-vde: fix runtime pm imbalance on error

[dinghao . liu]

Hi, Dan,

I agree the best solution is to fix __pm_runtime_resume(). But there are also 
many cases that assume pm_runtime_get_sync() will change PM usage 
counter on error. According to my static analysis results, the number of these 
"right" cases are larger. Adjusting __pm_runtime_resume() directly will 
introduce 
more new bugs. Therefore I think we should resolve the "bug" cases individually.

I think that Dmitry's patch is more reasonable than mine. 

Dinghao

"Dan Carpenter" <dan.carpen...@oracle.com>写道：
> On Wed, May 20, 2020 at 01:15:44PM +0300, Dmitry Osipenko wrote:
> > 20.05.2020 12:51, Dinghao Liu пишет:
> > > pm_runtime_get_sync() increments the runtime PM usage counter even
> > > it returns an error code. Thus a pairing decrement is needed on
> > > the error handling path to keep the counter balanced.
> > > 
> > > Signed-off-by: Dinghao Liu 
> > > ---
> > >  drivers/staging/media/tegra-vde/vde.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/media/tegra-vde/vde.c 
> > > b/drivers/staging/media/tegra-vde/vde.c
> > > index d3e63512a765..dd134a3a15c7 100644
> > > --- a/drivers/staging/media/tegra-vde/vde.c
> > > +++ b/drivers/staging/media/tegra-vde/vde.c
> > > @@ -777,7 +777,7 @@ static int tegra_vde_ioctl_decode_h264(struct 
> > > tegra_vde *vde,
> > >  
> > >   ret = pm_runtime_get_sync(dev);
> > >   if (ret < 0)
> > > - goto unlock;
> > > + goto put_runtime_pm;
> > >  
> > >   /*
> > >* We rely on the VDE registers reset value, otherwise VDE
> > > 
> > 
> > Hello Dinghao,
> > 
> > Thank you for the patch. I sent out a similar patch a week ago [1].
> > 
> > [1]
> > https://patchwork.ozlabs.org/project/linux-tegra/patch/20200514210847.9269-2-dig...@gmail.com/
> > 
> > The pm_runtime_put_noidle() should have the same effect as yours
> > variant, although my variant won't change the last_busy RPM time, which
> > I think is a bit more appropriate behavior.
> 
> I don't think either patch is correct.  The right thing to do is to fix
> __pm_runtime_resume() so it doesn't leak a reference count on error.
> 
> The problem is that a lot of functions don't check the return so
> possibly we are relying on that behavior.  We may need to introduce a
> new function which cleans up properly instead of leaking reference
> counts?
> 
> Also it's not documented that pm_runtime_get_sync() returns 1 sometimes
> on success so it leads to a few bugs.
> 
> drivers/gpu/drm/stm/ltdc.c: ret = pm_runtime_get_sync(ddev->dev);
> drivers/gpu/drm/stm/ltdc.c- if (ret) {
> --
> drivers/gpu/drm/stm/ltdc.c: ret = pm_runtime_get_sync(ddev->dev);
> drivers/gpu/drm/stm/ltdc.c- if (ret) {
> 
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c:  ret = 
> pm_runtime_get_sync(pm->dev);
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c-  if (ret)
> 
> drivers/media/platform/ti-vpe/cal.c:ret = pm_runtime_get_sync(&pdev->dev);
> drivers/media/platform/ti-vpe/cal.c-if (ret)
> 
> drivers/mfd/arizona-core.c: ret = 
> pm_runtime_get_sync(arizona->dev);
> drivers/mfd/arizona-core.c- if (ret != 0)
> 
> drivers/remoteproc/qcom_q6v5_adsp.c:ret = pm_runtime_get_sync(adsp->dev);
> drivers/remoteproc/qcom_q6v5_adsp.c-if (ret)
> 
> drivers/spi/spi-img-spfi.c: ret = pm_runtime_get_sync(dev);
> drivers/spi/spi-img-spfi.c- if (ret)
> 
> drivers/usb/dwc3/dwc3-pci.c:ret = pm_runtime_get_sync(&dwc3->dev);
> drivers/usb/dwc3/dwc3-pci.c-if (ret)
> 
> drivers/watchdog/rti_wdt.c: ret = pm_runtime_get_sync(dev);
> drivers/watchdog/rti_wdt.c- if (ret) {
> 
> regards,
> dan carpenter
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 99c7da112c95..e280991a977d 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1082,6 +1082,9 @@ int __pm_runtime_resume(struct device *dev, int 
> rpmflags)
>   retval = rpm_resume(dev, rpmflags);
>   spin_unlock_irqrestore(&dev->power.lock, flags);
>  
> + if (retval < 0 && rpmflags & RPM_GET_PUT)
> + atomic_dec(&dev->power.usage_count);
> +
>   return retval;
>  }
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] [v2] media: staging: tegra-vde: fix runtime pm imbalance on error

[Dinghao Liu]

pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.

Signed-off-by: Dinghao Liu 
---

Changelog:

v2: - Remove unused label 'unlock'
---
 drivers/staging/media/tegra-vde/vde.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/vde.c 
b/drivers/staging/media/tegra-vde/vde.c
index d3e63512a765..3fdf2cd0b99e 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -777,7 +777,7 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde 
*vde,
 
ret = pm_runtime_get_sync(dev);
if (ret < 0)
-   goto unlock;
+   goto put_runtime_pm;
 
/*
 * We rely on the VDE registers reset value, otherwise VDE
@@ -843,8 +843,6 @@ 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(&vde->lock);
 
 release_dpb_frames:
-- 
2.17.1

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

Re: Re: [PATCH] [v2] media: staging: tegra-vde: fix runtime pm imbalance on error

[dinghao . liu]

We need to make sure if pm_runtime_get_sync() is designed with
such behavior before modifying it.  

I received a response from Rafael when I commited a similar patch:
https://lkml.org/lkml/2020/5/20/1100
It seems that this behavior is intentional and needs to be kept.

Regards,
Dinghao

"Dan Carpenter" <dan.carpen...@oracle.com>写道：
> On Thu, May 21, 2020 at 02:27:45PM +0800, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > the call returns an error code. Thus a pairing decrement is needed
> > on the error handling path to keep the counter balanced.
> > 
> > Signed-off-by: Dinghao Liu 
> 
> Let's stop working around the bug in pm_runtime_get_sync() and write
> a replacement for it instead.
> 
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: Re: [PATCH] [v2] media: staging: tegra-vde: fix runtime pm imbalance on error

[dinghao . liu]

Sorry, I misunderstood your idea before. A new function is 
the best solution for this problem.

Regards,
Dinghao

"Dan Carpenter" 写道：
> On Thu, May 21, 2020 at 07:42:56PM +0800, dinghao@zju.edu.cn wrote:
> > We need to make sure if pm_runtime_get_sync() is designed with
> > such behavior before modifying it.  
> > 
> > I received a response from Rafael when I commited a similar patch:
> > https://lkml.org/lkml/2020/5/20/1100
> > It seems that this behavior is intentional and needs to be kept.
> 
> Yes.  This is why I have said twice or three times to not change
> pm_runtime_get_sync() but instead to write a replacement.
> 
> A large percent of the callers are buggy.  The pm_runtime_get_sync() is
> a -4 on Rusty's API scale.
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> One could argue that anything above a -4 is really a 2 if you read
> the implementation thouroughly enough...
> 
> regards,
> dan carpenter
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Dinghao Liu]

The variable authmode will keep uninitialized if neither if
statements used to initialize this variable are not triggered.
Then authmode may contain a garbage value and influence the
execution flow of this function.

Fix this by initializing it to zero.

Signed-off-by: Dinghao Liu 
---
 drivers/staging/rtl8188eu/core/rtw_mlme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 9de2d421f6b1..716f8d8a5c13 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
@@ -1711,7 +1711,7 @@ static int rtw_append_pmkid(struct adapter *Adapter, int 
iEntry, u8 *ie, uint ie
 
 int rtw_restruct_sec_ie(struct adapter *adapter, u8 *in_ie, u8 *out_ie, uint 
in_len)
 {
-   u8 authmode;
+   u8 authmode = 0;
uint ielength;
int iEntry;
struct mlme_priv *pmlmepriv = &adapter->mlmepriv;
-- 
2.17.1

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

Re: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[dinghao . liu]

> 
> Yes, in this routine, it would be possible for authmode to not be set; 
> however, 
> later code only compares it to either _WPA_IE_ID_ or _WPA2_IE_ID_. It is 
> never 
> used in a way that an unset value could make the program flow be different by 
> arbitrarily setting the value to zero. Thus your statement "Then authmode may 
> contain a garbage value and influence the execution flow of this function." 
> is 
> false.
> 

It's clear to me. Thank you for your advice.

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

Re: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[dinghao . liu]

> I review things in the order that they appear in my inbox so I hadn't
> seen Greg and Larry's comments.  You've now stumbled into an area of
> politics where you have conflicting reviews...  :P  Fortunately, we're
> all of us reasonable people.
> 
> I think your patch is correct in that it is what the original coder
> intended.  You just need to sell the patch correctly in the commit
> message.  The real danger of the original code would be if "authmode" is
> accidentally 0x30 or 0xdd just because it was uninitialized.  Setting it
> to zero ensures that it is not and it also matches how this is handled
> in the rtl8723bs driver.  This matches the original author's intention.
> 
> So:
> 
> 1) Re-write the commit message.
> 
> The variable authmode can be uninitialized.  The danger would be
> if it equals _WPA_IE_ID_ (0xdd) or _WPA2_IE_ID_ (0x33).  We can
> avoid this by setting it to zero instead.  This is the approach that
> was used in the rtl8723bs driver.
> 
> 2) Add a fixes tag.
>Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")
> 
> 3) Change the commit to Larry's style with the "else if" and "else".
>Sometimes people just disagree about style so it's easiest to go with
>whatever the maintainer (Larry) wants.  It's not worth debating one
>way or the other so just redo it.
> 
> Then resend.  Google for "how to send a v2 patch" to get the right
> format.
> 
> regards,
> dan carpenter
> 

Your advice is very helpful to me, thanks! I will prepare v2 patch and
resend it soon.

Regards,
Dinghao

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

[PATCH] [v2] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Dinghao Liu]

The variable authmode can be uninitialized. The danger would be if
it equals to _WPA_IE_ID_ (0xdd) or _WPA2_IE_ID_ (0x33). We can avoid
this by setting it to zero instead. This is the approach that was
used in the rtl8723bs driver.

Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")
Co-developed-by: Dan Carpenter 
Signed-off-by: Dan Carpenter 
Signed-off-by: Dinghao Liu 
---

Changelog:

v2: - Move the initialization after 'else' statement.
  Refine commit message.
---
 drivers/staging/rtl8188eu/core/rtw_mlme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 9de2d421f6b1..4f2abe1e14d5 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
@@ -1729,9 +1729,11 @@ int rtw_restruct_sec_ie(struct adapter *adapter, u8 
*in_ie, u8 *out_ie, uint in_
if ((ndisauthmode == Ndis802_11AuthModeWPA) ||
(ndisauthmode == Ndis802_11AuthModeWPAPSK))
authmode = _WPA_IE_ID_;
-   if ((ndisauthmode == Ndis802_11AuthModeWPA2) ||
+   else if ((ndisauthmode == Ndis802_11AuthModeWPA2) ||
(ndisauthmode == Ndis802_11AuthModeWPA2PSK))
authmode = _WPA2_IE_ID_;
+   else
+   authmode = 0x0;
 
if (check_fwstate(pmlmepriv, WIFI_UNDER_WPS)) {
memcpy(out_ie + ielength, psecuritypriv->wps_ie, 
psecuritypriv->wps_ie_len);
-- 
2.17.1

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

[PATCH] media: atomisp: fix memleak in ia_css_stream_create

[Dinghao Liu]

When aspect_ratio_crop_init() fails, curr_stream needs
to be freed just like what we've done in the following
error paths. However, current code is returning directly
and ends up leaking memory.

Signed-off-by: Dinghao Liu 
---
 drivers/staging/media/atomisp/pci/sh_css.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c 
b/drivers/staging/media/atomisp/pci/sh_css.c
index 54434c2dbaf9..8473e1437074 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -9521,7 +9521,7 @@ ia_css_stream_create(const struct ia_css_stream_config 
*stream_config,
if (err)
{
IA_CSS_LEAVE_ERR(err);
-   return err;
+   goto ERR;
}
 #endif
for (i = 0; i < num_pipes; i++)
-- 
2.17.1

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

[PATCH] vme: ca91cx42: fix memleak in ca91cx42_dma_list_add

[Dinghao Liu]

When we encounter invalid data width or address space,
entry should be freed just like what we've done in the
previous error paths.

Signed-off-by: Dinghao Liu 
---
 drivers/vme/bridges/vme_ca91cx42.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/vme/bridges/vme_ca91cx42.c 
b/drivers/vme/bridges/vme_ca91cx42.c
index ea938dc29c5e..1c464afd9d4e 100644
--- a/drivers/vme/bridges/vme_ca91cx42.c
+++ b/drivers/vme/bridges/vme_ca91cx42.c
@@ -1100,7 +1100,8 @@ static int ca91cx42_dma_list_add(struct vme_dma_list 
*list,
break;
default:
dev_err(dev, "Invalid data width\n");
-   return -EINVAL;
+   retval = -EINVAL;
+   goto err_cycle;
}
 
/* Setup address space */
@@ -1122,8 +1123,8 @@ static int ca91cx42_dma_list_add(struct vme_dma_list 
*list,
break;
default:
dev_err(dev, "Invalid address space\n");
-   return -EINVAL;
-   break;
+   retval = -EINVAL;
+   goto err_cycle;
}
 
if (vme_attr->cycle & VME_SUPER)
-- 
2.17.1

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