Hi Jason,
On Wed, 11 Apr 2018 09:09:25 -0400, Jason Andryuk wrote:
> On Tue, Apr 10, 2018 at 2:28 PM, Jean Delvare <jdelvare@xxxxxxx> wrote:
> > On Tue, 10 Apr 2018 11:57:00 -0400, Jason Andryuk wrote:
> >> Unfortunately, no. For a normal power off, the subsequent boot hangs
> >> as I was seeing before. However, if I rmmod the driver first and then
> >> power off, the subsequent boot does not hang. Maybe i801_remove is
> >> only called on rmmod, but not shutdown? Do we need to define
> >> shutdown() for struct pci_driver?
> >
> > I'm not sure. The current i2c-i801 driver code clearly assumes that the
> > remove function is called at shutdown time. But on the other hand there
> > must be a reason why a separate shutdown callback is supported at the
> > driver core level.
> >
> > That would be easy enough to test...
> >
> > ---
> > drivers/i2c/busses/i2c-i801.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > --- linux-4.15.orig/drivers/i2c/busses/i2c-i801.c 2018-04-10 19:01:57.085549960 +0200
> > +++ linux-4.15/drivers/i2c/busses/i2c-i801.c 2018-04-10 19:06:04.820155530 +0200
> > @@ -1700,6 +1700,16 @@ static void i801_remove(struct pci_dev *
> > */
> > }
> >
> > +static void i801_shutdown(struct pci_dev *dev)
> > +{
> > + struct i801_priv *priv = pci_get_drvdata(dev);
> > +
> > + dev_info(&dev->dev, "i801_shutdown is called\n");
> > +
> > + i801_disable_host_notify(priv);
> > + pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
> > +}
> > +
> > #ifdef CONFIG_PM
> > static int i801_suspend(struct device *dev)
> > {
> > @@ -1729,6 +1739,7 @@ static struct pci_driver i801_driver = {
> > .id_table = i801_ids,
> > .probe = i801_probe,
> > .remove = i801_remove,
> > + .shutdown = i801_shutdown,
> > .driver = {
> > .pm = &i801_pm_ops,
> > },
> >
> > Note that I can't see the log message in my own tests, but I think this
> > is because systemd gets down before the kernel drivers are shut down,
> > so nobody is there anymore to write the message to the disk. I would
> > have to setup a serial console to test properly, but I don't have time
> > to do that at the moment.
>
> This shutdown patch on top of the "i2c: i801: Save register SMBSLVCMD
> value only once" fixes the boot hang bug.
Yeah! \o/
Thanks for testing, I'll submit a v2 of the patch set later today for
immediate upstream inclusion.
> I also included the
> SMBSLVCMD debug print patch to log the values.
>
> At power-off, I see:
> i2c i2c-6: i801_enable_host_notify: SMBSLVCMD value before initialization: 01
My debug patch did not have this message. And it looks wrong to me...
value before initialization should be 00. And it must be, as that's the
value to which the register is restored below.
> i2c i2c-6: i801_enable_host_notify: SMBSLVCMD value after initialization: 01
This is from the i801_resume function, which oddly enough is called
before tearing down the device.
> i801_smbus 0000:00:1f.4: i801_shutdown is called
> i2c i2c-6: i801_disable_host_notify: SMBSLVCMD value at removal time: 01
> i2c i2c-6: i801_disable_host_notify: SMBSLVCMD reset to: 00
I'm curious how you managed to capture these... serial console, rsyslog?
--
Jean Delvare
SUSE L3 Support
