linux-kernel - Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Nov 2018 18:10:10 +0100
From:   Lubomir Rintel <lkundrak@...sk>
To:     Pavel Machek <pavel@....cz>
Cc:     Mark Brown <broonie@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        James Cameron <quozl@...top.org>,
        Sebastian Reichel <sre@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Eric Miao <eric.y.miao@...il.com>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Daniel Mack <daniel@...que.org>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        platform-driver-x86@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a
 struct

On Sun, 2018-11-04 at 15:37 +0100, Pavel Machek wrote:
> Hi!
> 
> > The global variables for private data are not too nice. I'd like some
> > more, and that would clutter the global name space even further.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@...sk>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> 
> Ok...
> 
> > -	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> > -	if (IS_ERR(olpc_bat)) {
> > -		ret = PTR_ERR(olpc_bat);
> > -		goto battery_failed;
> > -	}
> > +	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> > +	if (IS_ERR(data->olpc_bat))
> > +		return PTR_ERR(data->olpc_bat);
> >  
> > -	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > +	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	if (ret)
> > -		goto eeprom_failed;
> > +		return ret;
> >  
> > -	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> > +	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
> >  	if (ret)
> >  		goto error_failed;
> >  
> >  	if (olpc_ec_wakeup_available()) {
> > -		device_set_wakeup_capable(&olpc_ac->dev, true);
> > -		device_set_wakeup_capable(&olpc_bat->dev, true);
> > +		device_set_wakeup_capable(&data->olpc_ac->dev, true);
> > +		device_set_wakeup_capable(&data->olpc_bat->dev, true);
> >  	}
> >  
> >  	return 0;
> >  
> >  error_failed:
> > -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > -eeprom_failed:
> > -	power_supply_unregister(olpc_bat);
> > -battery_failed:
> > -	power_supply_unregister(olpc_ac);
> > +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	return ret;
> >  }
> 
> ...but you are changing error handling here, which is not mentioned in
> the changelog, and I'm nut sure you got it right.
> 
> Are you sure?

I can't see what's wrong. I'll split the priv structure and devm/error
handling changes into two separate patches as you're right they indeed
are somewhat unrelated.

If v2 (tomorrow or so) will still seem wrong to you I'd be thankful if
you could elaborate a bit more.
> 
> >  static int olpc_battery_remove(struct platform_device *pdev)
> >  {
> > -	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> > -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > -	power_supply_unregister(olpc_bat);
> > -	power_supply_unregister(olpc_ac);
> > +	struct olpc_battery_data *data = platform_get_drvdata(pdev);
> > +
> > +	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> > +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	return 0;
> >  }
> 
> Here too.
> 									Pavel

Cheers
Lubo