diff mbox series

[v2,01/10] appletalk: make localtalk and ppp support conditional

Message ID 20231011140225.253106-1-arnd@kernel.org (mailing list archive)
State Handled Elsewhere
Headers show
Series [v2,01/10] appletalk: make localtalk and ppp support conditional | expand

Commit Message

Arnd Bergmann Oct. 11, 2023, 2:02 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

The last localtalk driver is gone now, and ppp support was never fully
merged, but the code to support them for phase1 networking still calls
the deprecated .ndo_do_ioctl() helper.

In order to better isolate the localtalk and ppp portions of appletalk,
guard all of the corresponding code with CONFIG_DEV_APPLETALK checks,
including a preprocessor conditional that guards the internal ioctl calls.

This is currently all dead code and will now be left out of the
module since this Kconfig symbol is always undefined, but there are
plans to add a new driver for localtalk again in the future. When
that happens, the logic can be cleaned up to work properly without
the need for the ioctl.

Link: https://lore.kernel.org/lkml/790BA488-B6F6-41ED-96EF-2089EF1C043B@xhero.org/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: only add compile-time consitionals rather than removing localtalk
support entirely
---
 include/linux/atalk.h  |  1 -
 net/appletalk/Makefile |  3 ++-
 net/appletalk/aarp.c   | 24 +++++++++++++++---------
 net/appletalk/ddp.c    | 24 +++++++++++++-----------
 4 files changed, 30 insertions(+), 22 deletions(-)

Comments

Jiri Pirko Oct. 11, 2023, 3:04 p.m. UTC | #1 
Could you provide a cover letter for the set please?
Arnd Bergmann Oct. 11, 2023, 3:57 p.m. UTC | #2 
On Wed, Oct 11, 2023, at 17:04, Jiri Pirko wrote:
> Could you provide a cover letter for the set please?

Subject: [PATCH v2 00/10] remove final .ndo_do_ioctl references

The .ndo_do_ioctl() netdev operation used to be how one communicates
with a network driver from userspace, but since my previous cleanup [1],
it is purely internal to the kernel.

Removing the cops appletalk/localtalk driver made me revisit the
missing pieces from that older series, removing all the unused
implementations in wireless drivers as well as the two kernel-internal
callers in the ieee802154 and appletalk stacks.

One ethernet driver was already merged in the meantime that should
have used .ndo_eth_ioctl instead of .ndo_do_ioctl, so fix that as well.
With the complete removal, any future drivers making this mistake
cause build failures that are easier to spot.

[1] https://lore.kernel.org/netdev/20201106221743.3271965-1-arnd@kernel.org/

----
Hope that helps, I had commented on the cops removal about sending
this but of course not everyone here saw that. Let me know if I should
resend the patches together with the cover letter.

    Arnd
Jiri Pirko Oct. 12, 2023, 1:46 p.m. UTC | #3 
Wed, Oct 11, 2023 at 05:57:38PM CEST, arnd@arndb.de wrote:
>On Wed, Oct 11, 2023, at 17:04, Jiri Pirko wrote:
>> Could you provide a cover letter for the set please?
>
>Subject: [PATCH v2 00/10] remove final .ndo_do_ioctl references
>
>The .ndo_do_ioctl() netdev operation used to be how one communicates
>with a network driver from userspace, but since my previous cleanup [1],
>it is purely internal to the kernel.
>
>Removing the cops appletalk/localtalk driver made me revisit the
>missing pieces from that older series, removing all the unused
>implementations in wireless drivers as well as the two kernel-internal
>callers in the ieee802154 and appletalk stacks.
>
>One ethernet driver was already merged in the meantime that should
>have used .ndo_eth_ioctl instead of .ndo_do_ioctl, so fix that as well.
>With the complete removal, any future drivers making this mistake
>cause build failures that are easier to spot.

Looks fine.


>
>[1] https://lore.kernel.org/netdev/20201106221743.3271965-1-arnd@kernel.org/
>
>----
>Hope that helps, I had commented on the cops removal about sending
>this but of course not everyone here saw that. Let me know if I should
>resend the patches together with the cover letter.

Yes please. Thanks!


>
>    Arnd
Jakub Kicinski Oct. 14, 2023, 12:07 a.m. UTC | #4 
On Wed, 11 Oct 2023 17:57:38 +0200 Arnd Bergmann wrote:
> The .ndo_do_ioctl() netdev operation used to be how one communicates
> with a network driver from userspace, but since my previous cleanup [1],
> it is purely internal to the kernel.
> 
> Removing the cops appletalk/localtalk driver made me revisit the
> missing pieces from that older series, removing all the unused
> implementations in wireless drivers as well as the two kernel-internal
> callers in the ieee802154 and appletalk stacks.
> 
> One ethernet driver was already merged in the meantime that should
> have used .ndo_eth_ioctl instead of .ndo_do_ioctl, so fix that as well.
> With the complete removal, any future drivers making this mistake
> cause build failures that are easier to spot.
> 
> [1] https://lore.kernel.org/netdev/20201106221743.3271965-1-arnd@kernel.org/

Kalle, Johannes, do these apply for you?
I'm getting a small conflict on patch 8 and bigger one on patch 9.
If this applies for you maybe you can take it and flush out
wireless-next soon after?
Kalle Valo Oct. 14, 2023, 5 a.m. UTC | #5 
Jakub Kicinski <kuba@kernel.org> writes:

> On Wed, 11 Oct 2023 17:57:38 +0200 Arnd Bergmann wrote:
>> The .ndo_do_ioctl() netdev operation used to be how one communicates
>> with a network driver from userspace, but since my previous cleanup [1],
>> it is purely internal to the kernel.
>> 
>> Removing the cops appletalk/localtalk driver made me revisit the
>> missing pieces from that older series, removing all the unused
>> implementations in wireless drivers as well as the two kernel-internal
>> callers in the ieee802154 and appletalk stacks.
>> 
>> One ethernet driver was already merged in the meantime that should
>> have used .ndo_eth_ioctl instead of .ndo_do_ioctl, so fix that as well.
>> With the complete removal, any future drivers making this mistake
>> cause build failures that are easier to spot.
>> 
>> [1] https://lore.kernel.org/netdev/20201106221743.3271965-1-arnd@kernel.org/
>
> Kalle, Johannes, do these apply for you?
> I'm getting a small conflict on patch 8 and bigger one on patch 9.

Yes, 'git am -3' was able to solve the conflicts automatically and these
do apply to wireless-next.

> If this applies for you maybe you can take it and flush out
> wireless-next soon after?

Ok, we'll do that. Does next Wednesday sound soon enough? :)
Jakub Kicinski Oct. 18, 2023, 12:22 a.m. UTC | #6 
On Wed, 11 Oct 2023 16:02:16 +0200 Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The last localtalk driver is gone now, and ppp support was never fully
> merged, but the code to support them for phase1 networking still calls
> the deprecated .ndo_do_ioctl() helper.
> 
> In order to better isolate the localtalk and ppp portions of appletalk,
> guard all of the corresponding code with CONFIG_DEV_APPLETALK checks,
> including a preprocessor conditional that guards the internal ioctl calls.
> 
> This is currently all dead code and will now be left out of the
> module since this Kconfig symbol is always undefined, but there are
> plans to add a new driver for localtalk again in the future. When
> that happens, the logic can be cleaned up to work properly without
> the need for the ioctl.

Hi Arnd, the WiFi changes are now in net, could you rebase & repost?
Jakub Kicinski Oct. 18, 2023, 12:22 a.m. UTC | #7 
On Tue, 17 Oct 2023 17:22:02 -0700 Jakub Kicinski wrote:
> Hi Arnd, the WiFi changes are now in net, could you rebase & repost?

s/net/net-next/ to be more clear this time..
diff mbox series

Patch

diff --git a/include/linux/atalk.h b/include/linux/atalk.h
index a55bfc6567d01..2896f2ac9568e 100644
--- a/include/linux/atalk.h
+++ b/include/linux/atalk.h
@@ -121,7 +121,6 @@  static inline struct atalk_iface *atalk_find_dev(struct net_device *dev)
 #endif
 
 extern struct atalk_addr *atalk_find_dev_addr(struct net_device *dev);
-extern struct net_device *atrtr_get_dev(struct atalk_addr *sa);
 extern int		 aarp_send_ddp(struct net_device *dev,
 				       struct sk_buff *skb,
 				       struct atalk_addr *sa, void *hwaddr);
diff --git a/net/appletalk/Makefile b/net/appletalk/Makefile
index 33164d972d379..410d52f9113e2 100644
--- a/net/appletalk/Makefile
+++ b/net/appletalk/Makefile
@@ -5,6 +5,7 @@ 
 
 obj-$(CONFIG_ATALK) += appletalk.o
 
-appletalk-y			:= aarp.o ddp.o dev.o
+appletalk-y			:= aarp.o ddp.o
 appletalk-$(CONFIG_PROC_FS)	+= atalk_proc.o
 appletalk-$(CONFIG_SYSCTL)	+= sysctl_net_atalk.o
+appletalk-$(CONFIG_DEV_APPLETALK) += dev.o
diff --git a/net/appletalk/aarp.c b/net/appletalk/aarp.c
index 9fa0b246902be..b15f67293ac4c 100644
--- a/net/appletalk/aarp.c
+++ b/net/appletalk/aarp.c
@@ -438,14 +438,17 @@  static struct atalk_addr *__aarp_proxy_find(struct net_device *dev,
  */
 static void aarp_send_probe_phase1(struct atalk_iface *iface)
 {
+#if IS_ENABLED(CONFIG_DEV_APPLETALK)
 	struct ifreq atreq;
 	struct sockaddr_at *sa = (struct sockaddr_at *)&atreq.ifr_addr;
 	const struct net_device_ops *ops = iface->dev->netdev_ops;
 
 	sa->sat_addr.s_node = iface->address.s_node;
 	sa->sat_addr.s_net = ntohs(iface->address.s_net);
-
-	/* We pass the Net:Node to the drivers/cards by a Device ioctl. */
+	/*
+	 * We used to pass the address via device ioctl, this has to
+	 *  be rewritten if we bring back localtalk.
+	 */
 	if (!(ops->ndo_do_ioctl(iface->dev, &atreq, SIOCSIFADDR))) {
 		ops->ndo_do_ioctl(iface->dev, &atreq, SIOCGIFADDR);
 		if (iface->address.s_net != htons(sa->sat_addr.s_net) ||
@@ -455,13 +458,15 @@  static void aarp_send_probe_phase1(struct atalk_iface *iface)
 		iface->address.s_net  = htons(sa->sat_addr.s_net);
 		iface->address.s_node = sa->sat_addr.s_node;
 	}
+#endif
 }
 
 
 void aarp_probe_network(struct atalk_iface *atif)
 {
-	if (atif->dev->type == ARPHRD_LOCALTLK ||
-	    atif->dev->type == ARPHRD_PPP)
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK) &&
+	    (atif->dev->type == ARPHRD_LOCALTLK ||
+	     atif->dev->type == ARPHRD_PPP))
 		aarp_send_probe_phase1(atif);
 	else {
 		unsigned int count;
@@ -488,8 +493,9 @@  int aarp_proxy_probe_network(struct atalk_iface *atif, struct atalk_addr *sa)
 	 * we don't currently support LocalTalk or PPP for proxy AARP;
 	 * if someone wants to try and add it, have fun
 	 */
-	if (atif->dev->type == ARPHRD_LOCALTLK ||
-	    atif->dev->type == ARPHRD_PPP)
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK) &&
+	    (atif->dev->type == ARPHRD_LOCALTLK ||
+	     atif->dev->type == ARPHRD_PPP))
 		goto out;
 
 	/*
@@ -550,7 +556,8 @@  int aarp_send_ddp(struct net_device *dev, struct sk_buff *skb,
 	skb_reset_network_header(skb);
 
 	/* Check for LocalTalk first */
-	if (dev->type == ARPHRD_LOCALTLK) {
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK) &&
+	    dev->type == ARPHRD_LOCALTLK) {
 		struct atalk_addr *at = atalk_find_dev_addr(dev);
 		struct ddpehdr *ddp = (struct ddpehdr *)skb->data;
 		int ft = 2;
@@ -588,7 +595,7 @@  int aarp_send_ddp(struct net_device *dev, struct sk_buff *skb,
 	}
 
 	/* On a PPP link we neither compress nor aarp.  */
-	if (dev->type == ARPHRD_PPP) {
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK) && dev->type == ARPHRD_PPP) {
 		skb->protocol = htons(ETH_P_PPPTALK);
 		skb->dev = dev;
 		goto sendit;
@@ -674,7 +681,6 @@  int aarp_send_ddp(struct net_device *dev, struct sk_buff *skb,
 drop:
 	return NET_XMIT_DROP;
 }
-EXPORT_SYMBOL(aarp_send_ddp);
 
 /*
  *	An entry in the aarp unresolved queue has become resolved. Send
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 8978fb6212ffb..d4dc6a9fd3b6b 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -473,7 +473,7 @@  static struct atalk_route *atrtr_find(struct atalk_addr *target)
  * Given an AppleTalk network, find the device to use. This can be
  * a simple lookup.
  */
-struct net_device *atrtr_get_dev(struct atalk_addr *sa)
+static struct net_device *atrtr_get_dev(struct atalk_addr *sa)
 {
 	struct atalk_route *atr = atrtr_find(sa);
 	return atr ? atr->dev : NULL;
@@ -1947,10 +1947,6 @@  static struct packet_type ppptalk_packet_type __read_mostly = {
 
 static unsigned char ddp_snap_id[] = { 0x08, 0x00, 0x07, 0x80, 0x9B };
 
-/* Export symbols for use by drivers when AppleTalk is a module */
-EXPORT_SYMBOL(atrtr_get_dev);
-EXPORT_SYMBOL(atalk_find_dev_addr);
-
 /* Called by proto.c on kernel start up */
 static int __init atalk_init(void)
 {
@@ -1971,8 +1967,10 @@  static int __init atalk_init(void)
 		goto out_sock;
 	}
 
-	dev_add_pack(&ltalk_packet_type);
-	dev_add_pack(&ppptalk_packet_type);
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK)) {
+		dev_add_pack(&ltalk_packet_type);
+		dev_add_pack(&ppptalk_packet_type);
+	}
 
 	rc = register_netdevice_notifier(&ddp_notifier);
 	if (rc)
@@ -1998,8 +1996,10 @@  static int __init atalk_init(void)
 out_dev:
 	unregister_netdevice_notifier(&ddp_notifier);
 out_snap:
-	dev_remove_pack(&ppptalk_packet_type);
-	dev_remove_pack(&ltalk_packet_type);
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK)) {
+		dev_remove_pack(&ppptalk_packet_type);
+		dev_remove_pack(&ltalk_packet_type);
+	}
 	unregister_snap_client(ddp_dl);
 out_sock:
 	sock_unregister(PF_APPLETALK);
@@ -2026,8 +2026,10 @@  static void __exit atalk_exit(void)
 	atalk_proc_exit();
 	aarp_cleanup_module();	/* General aarp clean-up. */
 	unregister_netdevice_notifier(&ddp_notifier);
-	dev_remove_pack(&ltalk_packet_type);
-	dev_remove_pack(&ppptalk_packet_type);
+	if (IS_ENABLED(CONFIG_DEV_APPLETALK)) {
+		dev_remove_pack(&ltalk_packet_type);
+		dev_remove_pack(&ppptalk_packet_type);
+	}
 	unregister_snap_client(ddp_dl);
 	sock_unregister(PF_APPLETALK);
 	proto_unregister(&ddp_proto);