Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAMHSBOUtysO1Mj6stn1_wQ95s7mH5LuKD_Yd-4YZr7m7rCDkSQ@mail.gmail.com>
 <20180605004354.247410-1-egranata@google.com> <20180618063534.GG31141@dell>
In-Reply-To: <20180618063534.GG31141@dell>
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date:   Wed, 20 Jun 2018 13:57:44 -0700
Message-ID: <CAKdAkRSm0kRuNSjGd8rGuvK-k4j8SjCyqDSDjDyKTpP21hNP6Q@mail.gmail.com>
Subject: Re: [PATCH v3] cros_ec: cleanup dependency chain for cros_ec modules
To:     Lee Jones <lee.jones@linaro.org>, egranata@google.com
Cc:     lkml <linux-kernel@vger.kernel.org>, egranata@chromium.org,
        Benson Leung <bleung@chromium.org>,
        Olof Johansson <olof@lixom.net>,
        Gwendal Grignou <gwendal@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jun 17, 2018 at 11:37 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Mon, 04 Jun 2018, Enrico Granata wrote:
>
> > From: Enrico Granata <egranata@chromium.org>
> >
> > If one builds the Chrome EC support as modules, there is no explicit
> > dependency chain amongst the several modules (LPC, CHARDEV, CORE, ...)
> >
> > This makes it possible - for instance - to rmmod cros_ec_core, even though
> > cros_ec_dev actively uses it for data transport to the EC chip.
> >
> > This commit makes two changes in an attempt to address this:
> > a) moves cros_ec_proto.c as part of the CORE module; this removes the
> >    possibility of unloading cros_ec_core while cros_ec_dev is using it;
> > b) enables cros_ec_core to explicitly register a runtime dependency on
> >    the kernel module that is using its cmd and pkt transfer functions
> >
> > Series-to: LKML <linux-kernel@vger.kernel.org>
> > Series-cc: gwendal@google.com
> > Signed-off-by: Enrico Granata <egranata@chromium.org>
> > ---
> >  drivers/mfd/Kconfig                              |  1 -
> >  drivers/mfd/Makefile                             |  2 +-
> >  drivers/mfd/cros_ec.c                            | 18 ++++++++++++++++--
> >  drivers/mfd/cros_ec_i2c.c                        |  1 +
> >  drivers/{platform/chrome => mfd}/cros_ec_proto.c |  0
> >  drivers/mfd/cros_ec_spi.c                        |  1 +
> >  drivers/platform/chrome/Kconfig                  |  5 -----
> >  drivers/platform/chrome/Makefile                 |  1 -
> >  drivers/platform/chrome/cros_ec_lpc.c            |  1 +
> >  include/linux/mfd/cros_ec.h                      |  2 ++
> >  10 files changed, 22 insertions(+), 10 deletions(-)
> >  rename drivers/{platform/chrome => mfd}/cros_ec_proto.c (100%)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b860eb5aa194..a8daa2072ae6 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -194,7 +194,6 @@ config MFD_CROS_EC
> >       tristate "ChromeOS Embedded Controller"
> >       select MFD_CORE
> >       select CHROME_PLATFORMS
> > -     select CROS_EC_PROTO
> >       depends on X86 || ARM || ARM64 || COMPILE_TEST
> >       help
> >         If you say Y here you get support for the ChromeOS Embedded
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index d9d2cf0d32ef..20537bd27695 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -12,7 +12,7 @@ obj-$(CONFIG_MFD_SM501)             += sm501.o
> >  obj-$(CONFIG_MFD_ASIC3)              += asic3.o tmio_core.o
> >  obj-$(CONFIG_MFD_BCM590XX)   += bcm590xx.o
> >  obj-$(CONFIG_MFD_BD9571MWV)  += bd9571mwv.o
> > -cros_ec_core-objs            := cros_ec.o
> > +cros_ec_core-objs            := cros_ec.o cros_ec_proto.o
> >  cros_ec_core-$(CONFIG_ACPI)  += cros_ec_acpi_gpe.o
> >  obj-$(CONFIG_MFD_CROS_EC)    += cros_ec_core.o
> >  obj-$(CONFIG_MFD_CROS_EC_I2C)        += cros_ec_i2c.o
> > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > index d61024141e2b..9660dc0fc079 100644
> > --- a/drivers/mfd/cros_ec.c
> > +++ b/drivers/mfd/cros_ec.c
> > @@ -168,8 +168,19 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >        */
> >       err = cros_ec_sleep_event(ec_dev, 0);
> >       if (err < 0)
> > -             dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
> > -                     err);
> > +             dev_dbg(dev, "Error %d clearing sleep event to ec", err);
>
> This change is unrelated to the patch.
>
> > +     if (ec_dev->xfer_fcn_owner != NULL) {
>
> if (ec_dev->xfer_fcn_owner) ?
>
> > +             err = try_module_get(ec_dev->xfer_fcn_owner);
> > +             if (err < 0) {
>
> This logic is wrong.  try_module_get() returns a bool.

I do not think this is needed at all: the transport drivers (i2c, api)
are using cros_ec_register() provided by cros_ec core, which should
bump up refcount on cros_ec core module preventing it from unloading.
And unloading transport driver should unregister EC instance, stopping
IO.

If cros_ec_dev is not prepared to handle "dead" devices - it is issue
for cros_ec_dev to solve. By the way, it is not solvable my
disallowing module unloading: one can always tear down a device by
manually unbinding driver via sysfs.

Thanks.

-- 
Dmitry