linux-kernel - Re: [PATCH 1/2] i2c: add SMBus Host Notify support

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:	Tue, 7 Jul 2015 19:40:29 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:	Wolfram Sang <wsa@...-dreams.de>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] i2c: add SMBus Host Notify support

On Tue, 7 Jul 2015 10:23:33 -0400, Benjamin Tissoires wrote:
> Hi Jean,
> 
> On Jun 29 2015 or thereabouts, Jean Delvare wrote:
> > Hi Benjamin,
> > 
> > On Tue, 23 Jun 2015 14:58:18 -0400, Benjamin Tissoires wrote:
> > > SMBus Host Notify allows a slave device to act as a master on a bus to
> > > notify the host of an interrupt. The functionality is directly implemented
> > > in the firmware so we just export a toggle function and rely on the
> > > adapter to actually support this.
> > 
> > Why does it need to be toggled? I mean, if the controller supports it,
> > why don't we just enable the feature all the time?
> 
> That is definitively a solution. I however had once a problem with
> suspend/resume where i2c_i801 was not able to reset the Host Notify
> feature, but that was maybe a side effect of unloading/reloading the
> module tons of times during the session.
> 
> I will work on that and submit a v2 this week.
> 
> I still wonder if you think we should expose a KConfig parameter for
> this or not. I would prefer not to (because it is handled in hardware
> and people might screw up their input drivers), but I'd prefer have your
> opinion on this first.

What would the Kconfig option do exactly? As a general rule, the fewer
Kconfig options the better. We already have I2C_SMBUS for the SMBus
protocol specifics and Host Notify is one of these.

> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> > > ---
> > >  Documentation/i2c/smbus-protocol |  4 ++++
> > >  drivers/i2c/i2c-core.c           | 26 ++++++++++++++++++++++++++
> > >  include/linux/i2c.h              |  8 ++++++++
> > >  include/uapi/linux/i2c.h         |  1 +
> > >  4 files changed, 39 insertions(+)
> > > 
> > > diff --git a/Documentation/i2c/smbus-protocol b/Documentation/i2c/smbus-protocol
> > > index 6012b12..af4e4b9 100644
> > > --- a/Documentation/i2c/smbus-protocol
> > > +++ b/Documentation/i2c/smbus-protocol
> > > @@ -199,6 +199,10 @@ alerting device's address.
> > >  
> > >  [S] [HostAddr] [Wr] A [DevAddr] A [DataLow] A [DataHigh] A [P]
> > >  
> > > +I2C drivers for devices which can trigger SMBus Host Notify should call
> > > +i2c_smbus_toggle_host_notify() to enable SMBUS Host Notify on the adapter
> > > +and implement the optional alert() callback.
> > > +
> > >  
> > >  Packet Error Checking (PEC)
> > >  ===========================
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index 987c124..eaaf5b0 100644
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -2959,6 +2959,32 @@ int i2c_slave_unregister(struct i2c_client *client)
> > >  EXPORT_SYMBOL_GPL(i2c_slave_unregister);
> > >  #endif
> > >  
> > > +int i2c_smbus_toggle_host_notify(struct i2c_client *client, bool state)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!client)
> > > +		return -EINVAL;
> > > +
> > > +	if (!(client->flags & I2C_CLIENT_TEN)) {
> > > +		/* Enforce stricter address checking */
> > > +		ret = i2c_check_addr_validity(client->addr);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	if (!client->adapter->algo->toggle_smbus_host_notify)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	i2c_lock_adapter(client->adapter);
> > > +	ret = client->adapter->algo->toggle_smbus_host_notify(client->adapter,
> > > +							      state);
> > > +	i2c_unlock_adapter(client->adapter);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i2c_smbus_toggle_host_notify);
> > > +
> > 
> > As this is an SMBus feature, can't it go to drivers/i2c/i2c-smbus.c?
> 
> I asked myself this question, and I figured it was not possible because
> i2c-smbus consists in the smbus_alert i2c driver.
> 
> After re-reading the code, I see now that it could definitively goes
> there too. However, if we choose not to expose the toggle to the
> drivers, then there will be no need to make deep changes in i2c-smbus
> :).

The smbus_alert i2c driver was just the most simple way to implement
the SMBus Alert feature. It's not an actual device driver, nothing to
be afraid of.

> (...)
> Thanks for the review!

You're welcome. I wish I could do more but -ENOTIME :/

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/