diff mbox series

[1/3] mic: vop: fix a written error in MODULE_DEVICE_TABLE

Message ID 20200925073158.8238-2-sherry.sun@nxp.com (mailing list archive)
State New, archived
Headers show
Series Add module autoprobing support for vop and cosm driver | expand

Commit Message

Sherry Sun Sept. 25, 2020, 7:31 a.m. UTC 
For vop bus, the first parameter should be vop in MODULE_DEVICE_TABLE.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/misc/mic/vop/vop_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg Kroah-Hartman Sept. 27, 2020, 10:28 a.m. UTC | #1 
On Fri, Sep 25, 2020 at 03:31:56PM +0800, Sherry Sun wrote:
> For vop bus, the first parameter should be vop in MODULE_DEVICE_TABLE.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/misc/mic/vop/vop_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> index d609f0dc6124..589425fa78d4 100644
> --- a/drivers/misc/mic/vop/vop_main.c
> +++ b/drivers/misc/mic/vop/vop_main.c
> @@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
>  
>  module_vop_driver(vop_driver);
>  
> -MODULE_DEVICE_TABLE(mbus, id_table);
> +MODULE_DEVICE_TABLE(vop, id_table);
>  MODULE_AUTHOR("Intel Corporation");
>  MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
>  MODULE_LICENSE("GPL v2");

Doesn't this have to go _after_ the MODULE_DEVICE_TABLE(vop...) support,
which you add in patch 2 of this series?

Does this patch here break the build?  If not, how is it working?

And if you only have one vop driver, why do you need autoloading for it?

thanks,

greg k-h
Sherry Sun Sept. 27, 2020, 12:19 p.m. UTC | #2 
Hi Greg,

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 2020年9月27日 18:29
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: sudeep.dutt@intel.com; ashutosh.dixit@intel.com; arnd@arndb.de;
> masahiroy@kernel.org; michal.lkml@markovi.net; lee.jones@linaro.org;
> rikard.falkeborn@gmail.com; mst@redhat.co; bp@suse.de;
> jhugo@codeaurora.org; tglx@linutronix.de;
> manivannan.sadhasivam@linaro.org; mgross@linux.intel.com; pierre-
> louis.bossart@linux.intel.com; linux-kernel@vger.kernel.org; linux-
> kbuild@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/3] mic: vop: fix a written error in
> MODULE_DEVICE_TABLE
> 
> On Fri, Sep 25, 2020 at 03:31:56PM +0800, Sherry Sun wrote:
> > For vop bus, the first parameter should be vop in MODULE_DEVICE_TABLE.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/misc/mic/vop/vop_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mic/vop/vop_main.c
> > b/drivers/misc/mic/vop/vop_main.c index d609f0dc6124..589425fa78d4
> > 100644
> > --- a/drivers/misc/mic/vop/vop_main.c
> > +++ b/drivers/misc/mic/vop/vop_main.c
> > @@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
> >
> >  module_vop_driver(vop_driver);
> >
> > -MODULE_DEVICE_TABLE(mbus, id_table);
> > +MODULE_DEVICE_TABLE(vop, id_table);
> >  MODULE_AUTHOR("Intel Corporation");
> >  MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
> > MODULE_LICENSE("GPL v2");
> 
> Doesn't this have to go _after_ the MODULE_DEVICE_TABLE(vop...) support,
> which you add in patch 2 of this series?

Yes, this patch must be used in conjunction with Patch2.
But I think here may be a small bug, in order to distinguish it from the driver
autoloading support, make this a separate patch.

I can put this patch together with Patch2 if you think it might look more reasonable.

> 
> Does this patch here break the build?  If not, how is it working?
> 
> And if you only have one vop driver, why do you need autoloading for it?
> 
No, it doesn't break the build. But actually it won't work(autoloaded) when kernel boot and vop device appears.

Although we may only have one vop driver, but in the mic Kconfig, the intel mic/vop/cosm/scif drivers all
recommended to be built as modules, if we don't add autoloading for them, we may need modprobe them
one by one manually both on EP and RC side.

Obviously, for our use case, driver autoloading is more convenient.

Best regards
Sherry

> thanks,
> 
> greg k-h
Greg Kroah-Hartman Sept. 27, 2020, 12:31 p.m. UTC | #3 
On Sun, Sep 27, 2020 at 12:19:50PM +0000, Sherry Sun wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: 2020年9月27日 18:29
> > To: Sherry Sun <sherry.sun@nxp.com>
> > Cc: sudeep.dutt@intel.com; ashutosh.dixit@intel.com; arnd@arndb.de;
> > masahiroy@kernel.org; michal.lkml@markovi.net; lee.jones@linaro.org;
> > rikard.falkeborn@gmail.com; mst@redhat.co; bp@suse.de;
> > jhugo@codeaurora.org; tglx@linutronix.de;
> > manivannan.sadhasivam@linaro.org; mgross@linux.intel.com; pierre-
> > louis.bossart@linux.intel.com; linux-kernel@vger.kernel.org; linux-
> > kbuild@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH 1/3] mic: vop: fix a written error in
> > MODULE_DEVICE_TABLE
> > 
> > On Fri, Sep 25, 2020 at 03:31:56PM +0800, Sherry Sun wrote:
> > > For vop bus, the first parameter should be vop in MODULE_DEVICE_TABLE.
> > >
> > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > >  drivers/misc/mic/vop/vop_main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/misc/mic/vop/vop_main.c
> > > b/drivers/misc/mic/vop/vop_main.c index d609f0dc6124..589425fa78d4
> > > 100644
> > > --- a/drivers/misc/mic/vop/vop_main.c
> > > +++ b/drivers/misc/mic/vop/vop_main.c
> > > @@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
> > >
> > >  module_vop_driver(vop_driver);
> > >
> > > -MODULE_DEVICE_TABLE(mbus, id_table);
> > > +MODULE_DEVICE_TABLE(vop, id_table);
> > >  MODULE_AUTHOR("Intel Corporation");
> > >  MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
> > > MODULE_LICENSE("GPL v2");
> > 
> > Doesn't this have to go _after_ the MODULE_DEVICE_TABLE(vop...) support,
> > which you add in patch 2 of this series?
> 
> Yes, this patch must be used in conjunction with Patch2.
> But I think here may be a small bug, in order to distinguish it from the driver
> autoloading support, make this a separate patch.
> 
> I can put this patch together with Patch2 if you think it might look more reasonable.

How about _after_ patch 2, otherwise this patch will break the build,
right?

> > Does this patch here break the build?  If not, how is it working?
> > 
> > And if you only have one vop driver, why do you need autoloading for it?
> > 
> No, it doesn't break the build. But actually it won't work(autoloaded) when kernel boot and vop device appears.
> 
> Although we may only have one vop driver, but in the mic Kconfig, the intel mic/vop/cosm/scif drivers all
> recommended to be built as modules, if we don't add autoloading for them, we may need modprobe them
> one by one manually both on EP and RC side.
> 
> Obviously, for our use case, driver autoloading is more convenient.

Why are these all not "mic_SUFFIX" type drivers?  Why "vop" and "cosm"
and "scif"?

And if you only have 1 driver, then what would cause autoloading?

thanks,

greg k-h
Sherry Sun Sept. 27, 2020, 12:50 p.m. UTC | #4 
Hi Greg,

> On Sun, Sep 27, 2020 at 12:19:50PM +0000, Sherry Sun wrote:
> > Hi Greg,
> >
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: 2020年9月27日 18:29
> > > To: Sherry Sun <sherry.sun@nxp.com>
> > > Cc: sudeep.dutt@intel.com; ashutosh.dixit@intel.com; arnd@arndb.de;
> > > masahiroy@kernel.org; michal.lkml@markovi.net; lee.jones@linaro.org;
> > > rikard.falkeborn@gmail.com; mst@redhat.co; bp@suse.de;
> > > jhugo@codeaurora.org; tglx@linutronix.de;
> > > manivannan.sadhasivam@linaro.org; mgross@linux.intel.com; pierre-
> > > louis.bossart@linux.intel.com; linux-kernel@vger.kernel.org; linux-
> > > kbuild@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH 1/3] mic: vop: fix a written error in
> > > MODULE_DEVICE_TABLE
> > >
> > > On Fri, Sep 25, 2020 at 03:31:56PM +0800, Sherry Sun wrote:
> > > > For vop bus, the first parameter should be vop in
> MODULE_DEVICE_TABLE.
> > > >
> > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > ---
> > > >  drivers/misc/mic/vop/vop_main.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/misc/mic/vop/vop_main.c
> > > > b/drivers/misc/mic/vop/vop_main.c index
> d609f0dc6124..589425fa78d4
> > > > 100644
> > > > --- a/drivers/misc/mic/vop/vop_main.c
> > > > +++ b/drivers/misc/mic/vop/vop_main.c
> > > > @@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
> > > >
> > > >  module_vop_driver(vop_driver);
> > > >
> > > > -MODULE_DEVICE_TABLE(mbus, id_table);
> > > > +MODULE_DEVICE_TABLE(vop, id_table);
> > > >  MODULE_AUTHOR("Intel Corporation");
> > > >  MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
> > > > MODULE_LICENSE("GPL v2");
> > >
> > > Doesn't this have to go _after_ the MODULE_DEVICE_TABLE(vop...)
> > > support, which you add in patch 2 of this series?
> >
> > Yes, this patch must be used in conjunction with Patch2.
> > But I think here may be a small bug, in order to distinguish it from
> > the driver autoloading support, make this a separate patch.
> >
> > I can put this patch together with Patch2 if you think it might look more
> reasonable.
> 
> How about _after_ patch 2, otherwise this patch will break the build, right?

This change won't break the build, actually no matter what the first parameter
of MODULE_DEVICE_TABLE is, it won’t cause any build errors.

> 
> > > Does this patch here break the build?  If not, how is it working?
> > >
> > > And if you only have one vop driver, why do you need autoloading for it?
> > >
> > No, it doesn't break the build. But actually it won't work(autoloaded) when
> kernel boot and vop device appears.
> >
> > Although we may only have one vop driver, but in the mic Kconfig, the
> > intel mic/vop/cosm/scif drivers all recommended to be built as
> > modules, if we don't add autoloading for them, we may need modprobe
> them one by one manually both on EP and RC side.
> >
> > Obviously, for our use case, driver autoloading is more convenient.
> 
> Why are these all not "mic_SUFFIX" type drivers?  Why "vop" and "cosm"
> and "scif"?
> 

For VOP driver, it is designed to be a hardware independent Virtio Over PCIe (VOP) driver.
This is why we want to reuse it on i.MX platform. In theory, it can be applied to any platform.
With some changes, cosm driver also can be hardware independent.
So the names of them don’t use "mic_SUFFIX".

> And if you only have 1 driver, then what would cause autoloading?

As I understand it, if we add support for the driver autoloading like patch2,
driver will be autoloaded when the matched device appears.
Please correct me if I'm wrong.

Regards
Sherry

> 
> thanks,
> 
> greg k-h
diff mbox series

Patch

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index d609f0dc6124..589425fa78d4 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -796,7 +796,7 @@  static struct vop_driver vop_driver = {
 
 module_vop_driver(vop_driver);
 
-MODULE_DEVICE_TABLE(mbus, id_table);
+MODULE_DEVICE_TABLE(vop, id_table);
 MODULE_AUTHOR("Intel Corporation");
 MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
 MODULE_LICENSE("GPL v2");