On Wed, Feb 28, 2024 at 09:24:07PM +0300, Dan Carpenter wrote: > The dev->vqs[] array has "dev->vq_num" elements. It's allocated in > vduse_dev_init_vqs(). Thus, this > comparison needs to be >= to avoid > reading one element beyond the end of the array. > > Add an array_index_nospec() as well to prevent speculation issues. > > Fixes: 316ecd1346b0 ("vduse: Add file operation for mmap") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Thanks a lot! I assume this will be squashed in the relevant patch when that is re-spun. > --- > v2: add array_index_nospec() > v3: I accidentally corrupted v2. Try again. > > drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > index b7a1fb88c506..eb914084c650 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -1532,9 +1532,10 @@ static int vduse_dev_mmap(struct file *file, struct vm_area_struct *vma) > if ((vma->vm_flags & VM_SHARED) == 0) > return -EINVAL; > > - if (index > dev->vq_num) > + if (index >= dev->vq_num) > return -EINVAL; > > + index = array_index_nospec(index, dev->vq_num); > vq = dev->vqs[index]; > vaddr = vq->vdpa_reconnect_vaddr; > if (vaddr == 0) > -- > 2.43.0
Le 18/03/2024 à 04:21, Ratheesh Kannoth a écrit : > On 2024-03-16 at 15:46:10, Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote: >> UTILITY_NAME_LENGTH is 16. So better use the former when defining the >> 'utility_name' array. This makes the intent clearer when it is used around >> line 260. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> net/caif/cfctrl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/caif/cfctrl.c b/net/caif/cfctrl.c >> index 8480684f2762..b6d9462f92b9 100644 >> --- a/net/caif/cfctrl.c >> +++ b/net/caif/cfctrl.c >> @@ -206,7 +206,7 @@ int cfctrl_linkup_request(struct cflayer *layer, >> u8 tmp8; >> struct cfctrl_request_info *req; >> int ret; >> - char utility_name[16]; >> + char utility_name[UTILITY_NAME_LENGTH]; > Reverse xmas tree. Hi, I'll update and repost when net-next is reopened, but honestly, looking at this file, changing this to reverse xmas style won't change that much for the overall coding style! Moreover, as said by Dan, it is not really easy to understand the wishes of different maintainers. Should I have updated the lay-out, someone could have argued that patches should be 1 thing at a time. CJ > >> struct cfpkt *pkt; >> struct cflayer *dn = cfctrl->serv.layer.dn; >> >> -- >> 2.44.0 >> > >
On Mon, Mar 18, 2024 at 01:41:38PM -0500, Alex Elder wrote:
> On 3/18/24 12:58 PM, Markus Elfring wrote:
> > …
> > > +++ b/drivers/misc/mikrobus/mikrobus_core.c
> > …
> > > +static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
> > > + struct addon_board_info *board)
> > > +{
> > > + int ret;
> > > +
> > > + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> > > + ret = mikrobus_pinctrl_select(port, "pwm_default");
> > > + else
> > > + ret = mikrobus_pinctrl_select(port, "pwm_gpio");
> > …
> >
> > How do you think about to avoid the specification of a bit of duplicate source code here
> > by using conditional operator expressions?
> >
> > ret = mikrobus_pinctrl_select(port,
> > ((!board ||
> > board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> > ? "pwm_default"
> > : "pwm_gpio"));
>
> No.
>
> It's a complex enough bit of logic without trying to bury
> it inside the parameters passed to the function.
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
thanks,
greg k-h's patch email bot
On 3/18/24 12:58 PM, Markus Elfring wrote: > … >> +++ b/drivers/misc/mikrobus/mikrobus_core.c > … >> +static int mikrobus_pinctrl_setup(struct mikrobus_port *port, >> + struct addon_board_info *board) >> +{ >> + int ret; >> + >> + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) >> + ret = mikrobus_pinctrl_select(port, "pwm_default"); >> + else >> + ret = mikrobus_pinctrl_select(port, "pwm_gpio"); > … > > How do you think about to avoid the specification of a bit of duplicate source code here > by using conditional operator expressions? > > ret = mikrobus_pinctrl_select(port, > ((!board || > board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) > ? "pwm_default" > : "pwm_gpio")); No. It's a complex enough bit of logic without trying to bury it inside the parameters passed to the function. -Alex > > > Regards, > Markus >
… > +++ b/drivers/misc/mikrobus/mikrobus_core.h … > +#ifndef __MIKROBUS_H > +#define __MIKROBUS_H … I suggest to avoid the specification of leading underscores for include guards. See also: https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier#DCL37C.Donotdeclareordefineareservedidentifier-NoncompliantCodeExample%28IncludeGuard%29 Regards, Markus
… > +++ b/drivers/misc/mikrobus/mikrobus_core.c … > +static int mikrobus_pinctrl_setup(struct mikrobus_port *port, > + struct addon_board_info *board) > +{ > + int ret; > + > + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) > + ret = mikrobus_pinctrl_select(port, "pwm_default"); > + else > + ret = mikrobus_pinctrl_select(port, "pwm_gpio"); … How do you think about to avoid the specification of a bit of duplicate source code here by using conditional operator expressions? ret = mikrobus_pinctrl_select(port, ((!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) ? "pwm_default" : "pwm_gpio")); Regards, Markus
… > +++ b/drivers/misc/mikrobus/mikrobus_core.c … > +static int mikrobus_pinctrl_select(struct mikrobus_port *port, > + const char *pinctrl_selected) > +{ > + struct pinctrl_state *state; > + int ret; > + > + state = pinctrl_lookup_state(port->pinctrl, pinctrl_selected); > + if (IS_ERR(state)) { > + return dev_err_probe(&port->dev, PTR_ERR(state), > + "failed to find state %s", > + pinctrl_selected); > + } … I suggest to reconsider the need for extra curly brackets here. See also: Section “3) Placing Braces and Spaces” https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.8#n197 Regards, Markus
Hi Dan,
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't cause an
> issue at runtime because devm_kcalloc() won't allocate negative sizes.
> However, it's still worth fixing.
>
> Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
I've just tested on my board with the mp3309c chip, all is ok.
Thanks!
Tested-by: Flavio Suligoi <f.suligoi@asem.it>
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote:
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't
> cause an issue at runtime because devm_kcalloc() won't allocate negative
> sizes. However, it's still worth fixing.
>
> Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
On Mon, Mar 18, 2024 at 12:03:38PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 16, 2024 at 12:30:09PM +0300, Dan Carpenter wrote:
> > Goto the clean up path to clean up a couple clocks before returning
> > on this error path.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thank you for fixing this!
Btw, alternatively it may be switched to use devm_clk_get_enabled().
--
With Best Regards,
Andy Shevchenko
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote:
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't
> cause an issue at runtime because devm_kcalloc() won't allocate negative
> sizes. However, it's still worth fixing.
Agree.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
On Sat, Mar 16, 2024 at 12:30:09PM +0300, Dan Carpenter wrote:
> Goto the clean up path to clean up a couple clocks before returning
> on this error path.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Thank you for fixing this!
--
With Best Regards,
Andy Shevchenko
Sat, Mar 16, 2024 at 11:16:10AM CET, christophe.jaillet@wanadoo.fr wrote: >UTILITY_NAME_LENGTH is 16. So better use the former when defining the >'utility_name' array. This makes the intent clearer when it is used around >line 260. > >Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> This is net-next material, yet net-next is closed. https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle pw-bot: defer
On Mon, Mar 18, 2024 at 09:02:54AM +0100, Jiri Pirko wrote:
> Sat, Mar 16, 2024 at 10:46:03AM CET, dan.carpenter@linaro.org wrote:
> >The nh_grp_hw_stats_update() function doesn't always set "hw_stats_used"
> >so it could be used without being initialized. Set it to false.
> >
> >Fixes: 5072ae00aea4 ("net: nexthop: Expose nexthop group HW stats to user space")
> >Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >---
> > net/ipv4/nexthop.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> >index 74928a9d1aa4..c25bfdf4e25f 100644
> >--- a/net/ipv4/nexthop.c
> >+++ b/net/ipv4/nexthop.c
> >@@ -824,8 +824,8 @@ static int nla_put_nh_group_stats(struct sk_buff *skb, struct nexthop *nh,
> > u32 op_flags)
> > {
> > struct nh_group *nhg = rtnl_dereference(nh->nh_grp);
> >+ bool hw_stats_used = false;
> > struct nlattr *nest;
> >- bool hw_stats_used;
>
>
> Probably better to set this in one place and have:
> if (nexthop_notifiers_is_empty(net)) {
> *hw_stats_used = false;
> return 0;
> }
> in nh_grp_hw_stats_update().
>
Sure. Will do.
regards,
dan carpenter
On Mon, Mar 18, 2024 at 08:58:24AM +0100, Jiri Pirko wrote:
> Sat, Mar 16, 2024 at 10:44:40AM CET, dan.carpenter@linaro.org wrote:
> >Automatically cleaned up pointers need to be initialized before exiting
> >their scope. In this case, they need to be initialized to NULL before
> >any return statement.
> >
> >Fixes: 90f821d72e11 ("ice: avoid unnecessary devm_ usage")
> >Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >---
> > drivers/net/ethernet/intel/ice/ice_common.c | 4 ++--
> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> >index 4d8111aeb0ff..4b27d2bc2912 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_common.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_common.c
> >@@ -1002,8 +1002,8 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw)
> > */
> > int ice_init_hw(struct ice_hw *hw)
> > {
> >- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
> >- void *mac_buf __free(kfree);
> >+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
> >+ void *mac_buf __free(kfree) = NULL;
> > u16 mac_buf_len;
> > int status;
> >
>
> How about similar issues in:
> ice_set_fc()
> ice_cfg_phy_fec()
> ?
Yeah. Sorry, I'll resend. Smatch didn't warn about those bugs because
the sanity checks are the begining of the functions:
if (!pi || !aq_failures)
return -EINVAL;
are never true... It's the first time I've run into this as an issue.
regards,
dan carpenter
On Mon, Mar 18, 2024 at 08:51:33AM +0530, Ratheesh Kannoth wrote:
> On 2024-03-16 at 15:46:10, Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote:
> > UTILITY_NAME_LENGTH is 16. So better use the former when defining the
> > 'utility_name' array. This makes the intent clearer when it is used around
> > line 260.
> >
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > net/caif/cfctrl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/caif/cfctrl.c b/net/caif/cfctrl.c
> > index 8480684f2762..b6d9462f92b9 100644
> > --- a/net/caif/cfctrl.c
> > +++ b/net/caif/cfctrl.c
> > @@ -206,7 +206,7 @@ int cfctrl_linkup_request(struct cflayer *layer,
> > u8 tmp8;
> > struct cfctrl_request_info *req;
> > int ret;
> > - char utility_name[16];
> > + char utility_name[UTILITY_NAME_LENGTH];
> Reverse xmas tree.
>
It's always hard to know what to do when the original code isn't in the
correct format. Someone sent a patch last week which fixed a bug and
partially converted a declaration block into reverse Christmas tree...
regards,
dan carpenter
Sat, Mar 16, 2024 at 10:46:03AM CET, dan.carpenter@linaro.org wrote: >The nh_grp_hw_stats_update() function doesn't always set "hw_stats_used" >so it could be used without being initialized. Set it to false. > >Fixes: 5072ae00aea4 ("net: nexthop: Expose nexthop group HW stats to user space") >Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >--- > net/ipv4/nexthop.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c >index 74928a9d1aa4..c25bfdf4e25f 100644 >--- a/net/ipv4/nexthop.c >+++ b/net/ipv4/nexthop.c >@@ -824,8 +824,8 @@ static int nla_put_nh_group_stats(struct sk_buff *skb, struct nexthop *nh, > u32 op_flags) > { > struct nh_group *nhg = rtnl_dereference(nh->nh_grp); >+ bool hw_stats_used = false; > struct nlattr *nest; >- bool hw_stats_used; Probably better to set this in one place and have: if (nexthop_notifiers_is_empty(net)) { *hw_stats_used = false; return 0; } in nh_grp_hw_stats_update(). > int err; > int i; > >-- >2.43.0 > >
Sat, Mar 16, 2024 at 10:44:40AM CET, dan.carpenter@linaro.org wrote: >Automatically cleaned up pointers need to be initialized before exiting >their scope. In this case, they need to be initialized to NULL before >any return statement. > >Fixes: 90f821d72e11 ("ice: avoid unnecessary devm_ usage") >Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >--- > drivers/net/ethernet/intel/ice/ice_common.c | 4 ++-- > drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > >diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c >index 4d8111aeb0ff..4b27d2bc2912 100644 >--- a/drivers/net/ethernet/intel/ice/ice_common.c >+++ b/drivers/net/ethernet/intel/ice/ice_common.c >@@ -1002,8 +1002,8 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw) > */ > int ice_init_hw(struct ice_hw *hw) > { >- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree); >- void *mac_buf __free(kfree); >+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL; >+ void *mac_buf __free(kfree) = NULL; > u16 mac_buf_len; > int status; > How about similar issues in: ice_set_fc() ice_cfg_phy_fec() ? >diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c >index 255a9c8151b4..78b833b3e1d7 100644 >--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c >+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c >@@ -941,11 +941,11 @@ static u64 ice_loopback_test(struct net_device *netdev) > struct ice_netdev_priv *np = netdev_priv(netdev); > struct ice_vsi *orig_vsi = np->vsi, *test_vsi; > struct ice_pf *pf = orig_vsi->back; >+ u8 *tx_frame __free(kfree) = NULL; > u8 broadcast[ETH_ALEN], ret = 0; > int num_frames, valid_frames; > struct ice_tx_ring *tx_ring; > struct ice_rx_ring *rx_ring; >- u8 *tx_frame __free(kfree); > int i; > > netdev_info(netdev, "loopback test\n"); >-- >2.43.0 > >
On 2024-03-16 at 15:46:10, Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote: > UTILITY_NAME_LENGTH is 16. So better use the former when defining the > 'utility_name' array. This makes the intent clearer when it is used around > line 260. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > net/caif/cfctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/caif/cfctrl.c b/net/caif/cfctrl.c > index 8480684f2762..b6d9462f92b9 100644 > --- a/net/caif/cfctrl.c > +++ b/net/caif/cfctrl.c > @@ -206,7 +206,7 @@ int cfctrl_linkup_request(struct cflayer *layer, > u8 tmp8; > struct cfctrl_request_info *req; > int ret; > - char utility_name[16]; > + char utility_name[UTILITY_NAME_LENGTH]; Reverse xmas tree. > struct cfpkt *pkt; > struct cflayer *dn = cfctrl->serv.layer.dn; > > -- > 2.44.0 >
On 15/03/2024 17:19, Dan Carpenter wrote:
> Hello Tariq Toukan,
>
> Commit d3d057666090 ("net/mlx5: SD, Implement devcom communication
> and primary election") from Feb 14, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:221 sd_register()
> error: 'devcom' dereferencing possible ERR_PTR()
>
> drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> 206 static int sd_register(struct mlx5_core_dev *dev)
> 207 {
> 208 struct mlx5_devcom_comp_dev *devcom, *pos;
> 209 struct mlx5_core_dev *peer, *primary;
> 210 struct mlx5_sd *sd, *primary_sd;
> 211 int err, i;
> 212
> 213 sd = mlx5_get_sd(dev);
> 214 devcom = mlx5_devcom_register_component(dev->priv.devc, MLX5_DEVCOM_SD_GROUP,
> 215 sd->group_id, NULL, dev);
> 216 if (!devcom)
>
> The mlx5_devcom_register_component() function returns a mix of error
> pointers and NULL. It's not done really done correctly... Here is an
> explanation of how that normally works:
>
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
>
> mlx5_devcom_register_component() is not optional so it should only
> return error pointers.
>
> 217 return -ENOMEM;
> 218
> 219 sd->devcom = devcom;
> 220
> --> 221 if (mlx5_devcom_comp_get_size(devcom) != sd->host_buses)
> ^^^^^^
> Dead.
>
> 222 return 0;
>
> regards,
> dan carpenter
Hi Dan,
Thanks for your report!
I prepared a fix and will submit it soon.
Regards,
Tariq
On Sat, Mar 16, 2024 at 12:46:03PM +0300, Dan Carpenter wrote:
> The nh_grp_hw_stats_update() function doesn't always set "hw_stats_used"
> so it could be used without being initialized. Set it to false.
>
> Fixes: 5072ae00aea4 ("net: nexthop: Expose nexthop group HW stats to user space")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
On Sat, 16 Mar 2024 at 21:51, Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If lt9611uxc_audio_init() fails, some resources still need to be released
> before returning the error code.
>
> Use the existing error handling path.
>
> Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.
> ---
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
If lt9611uxc_audio_init() fails, some resources still need to be released before returning the error code. Use the existing error handling path. Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only. --- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index f4f593ad8f79..d0c77630a2f9 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -965,7 +965,11 @@ static int lt9611uxc_probe(struct i2c_client *client) } } - return lt9611uxc_audio_init(dev, lt9611uxc); + ret = lt9611uxc_audio_init(dev, lt9611uxc); + if (ret) + goto err_remove_bridge; + + return 0; err_remove_bridge: free_irq(client->irq, lt9611uxc); -- 2.44.0
If of_platform_populate() fails, we must shutdown the FPGA, as already done in the remove function. Fixes: 5b143d2a6ede ("bus: add driver for the Technologic Systems NBUS") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only. This patch is speculative and based on the output of one of my scripts that tries to spot calls in .remove function that are not also in the error handling path of the probe. I'm not familiar with the pwm_ API. I don't think that the locking in the remove function is needed here. Review with care. --- drivers/bus/ts-nbus.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c index baf22a82c47a..fa1bb9b78446 100644 --- a/drivers/bus/ts-nbus.c +++ b/drivers/bus/ts-nbus.c @@ -309,13 +309,19 @@ static int ts_nbus_probe(struct platform_device *pdev) dev_set_drvdata(dev, ts_nbus); ret = of_platform_populate(dev->of_node, NULL, NULL, dev); - if (ret < 0) - return dev_err_probe(dev, ret, - "failed to populate platform devices on bus\n"); + if (ret < 0) { + dev_err_probe(dev, ret, + "failed to populate platform devices on bus\n"); + goto err_disable_pwm; + } dev_info(dev, "initialized\n"); return 0; + +err_disable_pwm: + pwm_disable(ts_nbus->pwm); + return ret; } static void ts_nbus_remove(struct platform_device *pdev) -- 2.44.0
On 3/16/24 3:46 AM, Dan Carpenter wrote:
> The nh_grp_hw_stats_update() function doesn't always set "hw_stats_used"
> so it could be used without being initialized. Set it to false.
>
> Fixes: 5072ae00aea4 ("net: nexthop: Expose nexthop group HW stats to user space")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> net/ipv4/nexthop.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 74928a9d1aa4..c25bfdf4e25f 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -824,8 +824,8 @@ static int nla_put_nh_group_stats(struct sk_buff *skb, struct nexthop *nh,
> u32 op_flags)
> {
> struct nh_group *nhg = rtnl_dereference(nh->nh_grp);
> + bool hw_stats_used = false;
> struct nlattr *nest;
> - bool hw_stats_used;
> int err;
> int i;
>
Reviewed-by: David Ahern <dsahern@kernel.org>
The flag could be moved under
`if (op_flags & NHA_OP_FLAG_DUMP_HW_STATS ...`
as well.