linux-kernel - Re: [PATCH v2] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY

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] [day] [month] [year] [list]

Message-ID: <9f805ec3-1611-4682-831e-9499ba8284d5@notapiano>
Date: Tue, 16 Apr 2024 10:32:10 -0400
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Sebastian Reichel <sre@...nel.org>, kernel@...labora.com,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] power: supply: sbs-battery: Handle unsupported
 PROP_TIME_TO_EMPTY_NOW

On Tue, Apr 16, 2024 at 09:46:56AM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/24 21:05, Nícolas F. R. A. Prado ha scritto:
[..]
> > +
> > +static void sbs_update_quirks(struct sbs_info *chip)
> > +{
> > +	const char *model;
> > +	const char *manufacturer;
> > +	unsigned int i = 0;
> 
> Please reorder:
> 
> 	const char *manufacturer;
> 	const char *model;
> 	unsigned int i;
> 
> ...and please remove (like shown, of course) the double initialization of the
> `i` variable, as you're initializing it again later in your for loop.

Ack.

> 
> > +
> > +	/* reset quirks from battery before the hot-plug event */
> > +	chip->quirks = 0;
> > +
> > +	manufacturer = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MANUFACTURER);
> > +	model = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> > +	if (IS_ERR(manufacturer) || IS_ERR(model)) {
> > +		dev_warn(&chip->client->dev, "Couldn't read manufacturer and model to set quirks\n");
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sbs_quirks); i++) {
> > +		if (strcmp(manufacturer, sbs_quirks[i].manufacturer))
> > +			continue;
> > +		if (strcmp(model, sbs_quirks[i].model))
> > +			continue;
> > +		chip->quirks |= sbs_quirks[i].flags;
> > +	}
> > +
> > +	if (chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW)
> > +		dev_info(&chip->client->dev, "Added quirk disabling TIME_TO_EMPTY_NOW\n");
> 
> I don't really expect many quirks, but having a dev_info() print for every
> quirk that gets set would make this driver too chatty, IMO.
> 
> Please, either turn that into a dev_dbg() or print a mask of the quirks .. or both.

I wouldn't make this debug as it's pretty important information to be
reported. I'd be ok with having a single message listing all the quirks, but at
the same time I feel like this would be trying to fix a problem that we don't
know will ever exist (one SBS battery that has many quirks). I propose we keep
it the way it is - a nice clear message of what has happened - and adapt in the
future if it turns out to be too much information.

Thanks,
Nícolas