linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: dev: Add API to check net_dev readiness
@ 2020-07-26 19:37 Ilia Lin
  2020-07-26 19:45 ` Andrew Lunn
  0 siblings, 1 reply; 8+ messages in thread
From: Ilia Lin @ 2020-07-26 19:37 UTC (permalink / raw)
  To: davem, kuba, jiri, edumazet, ap420073, xiyou.wangcong, maximmi, ilia.lin
  Cc: netdev, linux-kernel

From: Ilia Lin <ilia.lin@kernel.org>

Add an API that returns true, if the net_dev_init was already called,
and the driver was initialized.

Some early drivers, that are initialized during the subsys_initcall
may try accessing the net_dev or NAPI APIs before the net_dev_init,
and will encounter a kernel bug. This API provides a way to handle
this and manage by deferring or by other way.

Signed-off-by: Ilia Lin <ilia.lin@kernel.org>
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 98d290c..d17d364 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2600,6 +2600,8 @@ enum netdev_cmd {
 };
 const char *netdev_cmd_to_name(enum netdev_cmd cmd);
 
+bool is_net_dev_initialized(void);
+
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
 int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 316349f..1b50488 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1793,6 +1793,23 @@ static void call_netdevice_unregister_net_notifiers(struct notifier_block *nb,
 static int dev_boot_phase = 1;
 
 /**
+ * is_net_dev_initialized - check, whether the net_dev was
+ * initialized
+ *
+ * Returns true, if the net_dev_init was already called, and
+ * the driver is initialized.
+ *
+ * This is useful for early drivers trying to call net_dev and
+ * NAPI APIs
+ */
+
+bool is_net_dev_initialized(void)
+{
+       return !(bool)dev_boot_phase;
+}
+EXPORT_SYMBOL(is_net_dev_initialized);
+
+/**
  * register_netdevice_notifier - register a network notifier block
  * @nb: notifier
  *


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-07-26 19:37 [PATCH] net: dev: Add API to check net_dev readiness Ilia Lin
@ 2020-07-26 19:45 ` Andrew Lunn
  2020-07-27 17:32   ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2020-07-26 19:45 UTC (permalink / raw)
  To: Ilia Lin
  Cc: davem, kuba, jiri, edumazet, ap420073, xiyou.wangcong, maximmi,
	ilia.lin, netdev, linux-kernel

On Sun, Jul 26, 2020 at 10:37:54PM +0300, Ilia Lin wrote:
> From: Ilia Lin <ilia.lin@kernel.org>
> 
> Add an API that returns true, if the net_dev_init was already called,
> and the driver was initialized.
> 
> Some early drivers, that are initialized during the subsys_initcall
> may try accessing the net_dev or NAPI APIs before the net_dev_init,
> and will encounter a kernel bug. This API provides a way to handle
> this and manage by deferring or by other way.

Hi Ilia

You need to include a user of this new API.

I also have to wonder why a network device driver is being probed the
subsys_initcall.

    Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-07-26 19:45 ` Andrew Lunn
@ 2020-07-27 17:32   ` David Miller
  2020-08-04 17:47     ` Ilia Lin
  2020-08-04 18:42     ` Ilia Lin
  0 siblings, 2 replies; 8+ messages in thread
From: David Miller @ 2020-07-27 17:32 UTC (permalink / raw)
  To: andrew
  Cc: ilial, kuba, jiri, edumazet, ap420073, xiyou.wangcong, maximmi,
	ilia.lin, netdev, linux-kernel

From: Andrew Lunn <andrew@lunn.ch>
Date: Sun, 26 Jul 2020 21:45:28 +0200

> I also have to wonder why a network device driver is being probed the
> subsys_initcall.

This makes me wonder how this interface could even be useful.  The
only way to fix the problem is to change when the device is probed,
which would mean changing which initcall it uses.  So at run time,
this information can't do much.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-07-27 17:32   ` David Miller
@ 2020-08-04 17:47     ` Ilia Lin
  2020-08-04 19:24       ` Andrew Lunn
  2020-08-04 18:42     ` Ilia Lin
  1 sibling, 1 reply; 8+ messages in thread
From: Ilia Lin @ 2020-08-04 17:47 UTC (permalink / raw)
  To: David Miller
  Cc: andrew, ilial, kuba, jiri, edumazet, ap420073, xiyou.wangcong,
	maximmi, Ilia Lin, netdev, open list

Hi Andrew and David,

Thank you for your comments!

The client driver is still work in progress, but it can be seen here:
https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/platform/msm/ipa/ipa_api.c#n3842

For HW performance reasons, it has to be in subsys_initcall.

Here is the register_netdev call:
https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/platform/msm/ipa/ipa_v3/rmnet_ipa.c#n2497

And it is going to be in the subsys_initcall as well.

Thanks,
Ilia



On Mon, Jul 27, 2020 at 8:32 PM David Miller <davem@davemloft.net> wrote:
>
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Sun, 26 Jul 2020 21:45:28 +0200
>
> > I also have to wonder why a network device driver is being probed the
> > subsys_initcall.
>
> This makes me wonder how this interface could even be useful.  The
> only way to fix the problem is to change when the device is probed,
> which would mean changing which initcall it uses.  So at run time,
> this information can't do much.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-07-27 17:32   ` David Miller
  2020-08-04 17:47     ` Ilia Lin
@ 2020-08-04 18:42     ` Ilia Lin
  1 sibling, 0 replies; 8+ messages in thread
From: Ilia Lin @ 2020-08-04 18:42 UTC (permalink / raw)
  To: David Miller
  Cc: andrew, kuba, jiri, edumazet, ap420073, xiyou.wangcong, maximmi,
	Ilia Lin, netdev, open list

Hi Andrew and David,

Thank you for your comments!

The client driver is still work in progress, but it can be seen here:
https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/platform/msm/ipa/ipa_api.c#n3842

For HW performance reasons, it has to be in subsys_initcall.

Here is the register_netdev call:
https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/platform/msm/ipa/ipa_v3/rmnet_ipa.c#n2497

And it is going to be in the subsys_initcall as well.

Thanks,
Ilia

On Mon, Jul 27, 2020 at 8:32 PM David Miller <davem@davemloft.net> wrote:
>
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Sun, 26 Jul 2020 21:45:28 +0200
>
> > I also have to wonder why a network device driver is being probed the
> > subsys_initcall.
>
> This makes me wonder how this interface could even be useful.  The
> only way to fix the problem is to change when the device is probed,
> which would mean changing which initcall it uses.  So at run time,
> this information can't do much.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-08-04 17:47     ` Ilia Lin
@ 2020-08-04 19:24       ` Andrew Lunn
  2020-08-05 10:31         ` Ilia Lin
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2020-08-04 19:24 UTC (permalink / raw)
  To: Ilia Lin
  Cc: David Miller, ilial, kuba, jiri, edumazet, ap420073,
	xiyou.wangcong, maximmi, Ilia Lin, netdev, open list

On Tue, Aug 04, 2020 at 08:47:18PM +0300, Ilia Lin wrote:
> Hi Andrew and David,

Hi Ilia

Please don't top post.

> 
> Thank you for your comments!
> 
> The client driver is still work in progress, but it can be seen here:
> https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/platform/msm/ipa/ipa_api.c#n3842
> 
> For HW performance reasons, it has to be in subsys_initcall.

Well, until the user of this new API is ready, we will not accept the
patch.

You also need to explain "For HW performance reasons". Why is this
driver special that it can do things which no over driver does?

And you should really be working on net-next, not this dead kernel
version, if you want to get merged into mainline.

Network drivers do not belong is drivers/platform. There is also ready
a drivers/net/ipa, so i assume you will move there.

  Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-08-04 19:24       ` Andrew Lunn
@ 2020-08-05 10:31         ` Ilia Lin
  2020-08-05 13:14           ` Andrew Lunn
  0 siblings, 1 reply; 8+ messages in thread
From: Ilia Lin @ 2020-08-05 10:31 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: David Miller, ilial, kuba, jiri, edumazet, ap420073,
	xiyou.wangcong, maximmi, Ilia Lin, netdev, open list

My comments inline.

Thanks,
Ilia Lin

On Tue, Aug 4, 2020 at 10:24 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 04, 2020 at 08:47:18PM +0300, Ilia Lin wrote:
> > Hi Andrew and David,
>
> Hi Ilia
>
> Please don't top post.
>
> >
> > Thank you for your comments!
> >
> > The client driver is still work in progress, but it can be seen here:
> > https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/platform/msm/ipa/ipa_api.c#n3842
> >
> > For HW performance reasons, it has to be in subsys_initcall.
>
> Well, until the user of this new API is ready, we will not accept the
> patch.
OK, but once we submit the change in the driver, is it good to go?
>
> You also need to explain "For HW performance reasons". Why is this
> driver special that it can do things which no over driver does?
There are very strict KPI requirements. E.g. automotive rear mirror
must be online 3 sec since boot.
>
> And you should really be working on net-next, not this dead kernel
> version, if you want to get merged into mainline.
Of course, the upstream submition will be done on the mainline. I just
gave an example of an existing product driver.
>
> Network drivers do not belong is drivers/platform. There is also ready
> a drivers/net/ipa, so i assume you will move there.
Sure, the driver in the drivers/net/ipa is the one that will be
updated in the future.
>
>   Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: dev: Add API to check net_dev readiness
  2020-08-05 10:31         ` Ilia Lin
@ 2020-08-05 13:14           ` Andrew Lunn
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2020-08-05 13:14 UTC (permalink / raw)
  To: Ilia Lin
  Cc: David Miller, ilial, kuba, jiri, edumazet, ap420073,
	xiyou.wangcong, maximmi, netdev, open list

> > Well, until the user of this new API is ready, we will not accept the
> > patch.
> OK, but once we submit the change in the driver, is it good to go?

No. You really do need to explain why it is needed, and why it is
safe.

> > You also need to explain "For HW performance reasons". Why is this
> > driver special that it can do things which no over driver does?
> There are very strict KPI requirements. E.g. automotive rear mirror
> must be online 3 sec since boot.

Which does not explain why this driver is special. What you really
should be thinking about is having the required drivers for this use
case built in, and the rest as modules. Get your time critical parts
running, and then load the rest of the driver moduless and kick off
additional user space in a second phase.

You are breaking all sorts of assumptions by loading network drivers
before the stack is ready. You need to expect all sorts of nasty bugs.
If this was just in your vendor kernel, we would not care too much, it
is your problem to solve. But by doing this in mainline, you are
setting a precedent for others to do it as well. And then we really do
need to care about the broken assumptions. I doubt we are ready to
allow this.

      Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-08-05 20:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-26 19:37 [PATCH] net: dev: Add API to check net_dev readiness Ilia Lin
2020-07-26 19:45 ` Andrew Lunn
2020-07-27 17:32   ` David Miller
2020-08-04 17:47     ` Ilia Lin
2020-08-04 19:24       ` Andrew Lunn
2020-08-05 10:31         ` Ilia Lin
2020-08-05 13:14           ` Andrew Lunn
2020-08-04 18:42     ` Ilia Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).