[PATCH] make NatFeat drivers bool, not tristate

[PATCH] make NatFeat drivers bool, not tristate

[Thorsten Glaser]

- nfblock: doesn't create /dev/nfhd8p1 device node after modprobe
- nfcon: is totally useless as module, almost only used during boot
- nfeth: says "No such device" upon insmod

Signed-off-by: Thorsten Glaser <tg@debian.org>
---
 arch/m68k/Kconfig.devices |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/m68k/Kconfig.devices b/arch/m68k/Kconfig.devices
index d163991..a306c50 100644
--- a/arch/m68k/Kconfig.devices
+++ b/arch/m68k/Kconfig.devices
@@ -32,7 +32,7 @@ config NATFEAT
 	  access to a disk image as /dev/hda.
 
 config NFBLOCK
-	tristate "NatFeat block device support"
+	bool "NatFeat block device support"
 	depends on BLOCK && NATFEAT
 	help
 	  Say Y to include support for the ARAnyM NatFeat block device
@@ -40,7 +40,7 @@ config NFBLOCK
 	  the hardware emulation.
 
 config NFCON
-	tristate "NatFeat console driver"
+	bool "NatFeat console driver"
 	depends on TTY && NATFEAT
 	help
 	  Say Y to include support for the ARAnyM NatFeat console driver
@@ -48,7 +48,7 @@ config NFCON
 	  output of ARAnyM.
 
 config NFETH
-	tristate "NatFeat Ethernet support"
+	bool "NatFeat Ethernet support"
 	depends on ETHERNET && NATFEAT
 	help
 	  Say Y to include support for the ARAnyM NatFeat network device
-- 
1.7.9

Re: [PATCH] make NatFeat drivers bool, not tristate

[Andreas Schwab]

Thorsten Glaser <tg@debian.org> writes:

> - nfblock: doesn't create /dev/nfhd8p1 device node after modprobe
> - nfcon: is totally useless as module, almost only used during boot
> - nfeth: says "No such device" upon insmod

The problem is that aranym expects the parameters of the natfeat calls
to be physical addresses.  The nf_get_id calls use literal strings as
parameters which only works when the drivers are builtin because phys ==
virt then.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] make NatFeat drivers bool, not tristate

[Thorsten Glaser]

Andreas Schwab dixit:

>The problem is that aranym expects the parameters of the natfeat calls
>to be physical addresses.  The nf_get_id calls use literal strings as
>parameters which only works when the drivers are builtin because phys ==
>virt then.

Ah, right, there was that issue… so this patch should probably go in?

bye,
//mirabilos
-- 
17:08⎜«Vutral» früher gabs keine packenden smartphones und so
17:08⎜«Vutral» heute gibts frauen die sind facebooksüchtig
17:10⎜«Vutral» aber auch traurig; früher warst du als nerd voll am arsch
17:10⎜«Vutral» heute bist du als nerd der einzige der wirklich damit klarkommt

Re: [PATCH] make NatFeat drivers bool, not tristate

[Thorsten Glaser]

Thorsten Glaser <tg <at> mirbsd.de> writes:

> Andreas Schwab dixit:
> 
> >The problem is that aranym expects the parameters of the natfeat calls
> >to be physical addresses.  The nf_get_id calls use literal strings as
> >parameters which only works when the drivers are builtin because phys ==
> >virt then.
> 
> Ah, right, there was that issue… so this patch should probably go in?

At first, anyway.

Maybe if CONFIG_NATFEAT is selected (it’s bool), some bounce buffer could
be allocated and memlocked, which the other NF* drivers can then use.
(If NatFeat is not found during boot, no memory should be allocated.)

Especially if we had more NF* drivers in the future (hostfs could be made
into something, maybe with something like umsdos on top for Unix file
permissions? And I’m hoping for a bidirectional “virtio/serial” console…)
this could be useful sharing, and not needing to have everything else
in the kernel statically.

Well, if someone has time and wants to write this.

bye,
//mirabilos

Re: [PATCH] make NatFeat drivers bool, not tristate

[Geert Uytterhoeven]

On Wed, 24 Jul 2013, Thorsten Glaser wrote:
> Thorsten Glaser <tg <at> mirbsd.de> writes:
> > Andreas Schwab dixit:
> > >The problem is that aranym expects the parameters of the natfeat calls
> > >to be physical addresses.  The nf_get_id calls use literal strings as
> > >parameters which only works when the drivers are builtin because phys ==
> > >virt then.

And fixing the conversion is non-trivial, as module memory is allocated using
vmalloc(), which may be non-contiguous in physical memory space, right?

> > Ah, right, there was that issue… so this patch should probably go in?
> 
> At first, anyway.

I prefer a fix to go in instead :-)

> Maybe if CONFIG_NATFEAT is selected (it’s bool), some bounce buffer could
> be allocated and memlocked, which the other NF* drivers can then use.
> (If NatFeat is not found during boot, no memory should be allocated.)
> 
> Especially if we had more NF* drivers in the future (hostfs could be made
> into something, maybe with something like umsdos on top for Unix file
> permissions? And I’m hoping for a bidirectional “virtio/serial” console…)
> this could be useful sharing, and not needing to have everything else
> in the kernel statically.
> 
> Well, if someone has time and wants to write this.

It doesn't have to be that complex...

The patch below adds a wrapper that copies the (supposedly short) name to
the kernel stack before calling into ARAnyM.
Does this make modular NatFeat drivers work for you?
It doesn't break the built-in drivers for me.

ARAnyM hackers: What's the maximum size of ID names?

diff --git a/arch/m68k/emu/natfeat.c b/arch/m68k/emu/natfeat.c
index cb574ae..98655ef 100644
--- a/arch/m68k/emu/natfeat.c
+++ b/arch/m68k/emu/natfeat.c
@@ -19,9 +19,11 @@
 #include <asm/machdep.h>
 #include <asm/natfeat.h>
 
+extern long nf_get_id2(const char *feature_name);
+
 asm("\n"
-"	.global nf_get_id,nf_call\n"
-"nf_get_id:\n"
+"	.global nf_get_id2,nf_call\n"
+"nf_get_id2:\n"
 "	.short	0x7300\n"
 "	rts\n"
 "nf_call:\n"
@@ -30,12 +32,24 @@ asm("\n"
 "1:	moveq.l	#0,%d0\n"
 "	rts\n"
 "	.section __ex_table,\"a\"\n"
-"	.long	nf_get_id,1b\n"
+"	.long	nf_get_id2,1b\n"
 "	.long	nf_call,1b\n"
 "	.previous");
-EXPORT_SYMBOL_GPL(nf_get_id);
 EXPORT_SYMBOL_GPL(nf_call);
 
+long nf_get_id(const char *feature_name)
+{
+	char name_copy[32]; // FIXME maximum size of ID names?
+	size_t n;
+
+	n = strlcpy(name_copy, feature_name, sizeof(name_copy));
+	if (n >= sizeof(name_copy))
+		return 0;
+
+	return nf_get_id2(name_copy);
+}
+EXPORT_SYMBOL_GPL(nf_get_id);
+
 void nfprint(const char *fmt, ...)
 {
 	static char buf[256];

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] make NatFeat drivers bool, not tristate

[Petr Stehlik]

Geert Uytterhoeven píše v St 24. 07. 2013 v 16:56 +0200:
> > > >The problem is that aranym expects the parameters of the natfeat calls
> > > >to be physical addresses.  The nf_get_id calls use literal strings as
> > > >parameters which only works when the drivers are builtin because phys ==
> > > >virt then.
> 
> And fixing the conversion is non-trivial, as module memory is allocated using
> vmalloc(), which may be non-contiguous in physical memory space, right?

Sounds like the original design decision of NatFeats to use ASCII names
instead of cryptic integer codes wasn't that smart.

> ARAnyM hackers: What's the maximum size of ID names?

There isn't any given limit AFAIK, it's based on a common sense when
creating a new NatFeat name.

Petr

Re: [PATCH] make NatFeat drivers bool, not tristate

[Geert Uytterhoeven]

On Wed, Jul 24, 2013 at 5:02 PM, Petr Stehlik <pstehlik@sophics.cz> wrote:
>> ARAnyM hackers: What's the maximum size of ID names?
>
> There isn't any given limit AFAIK, it's based on a common sense when
> creating a new NatFeat name.

Understood :-) What's the size of the longest name in use?
Is 32 OK?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] make NatFeat drivers bool, not tristate

[Petr Stehlik]

Geert Uytterhoeven píše v St 24. 07. 2013 v 18:19 +0200:
> On Wed, Jul 24, 2013 at 5:02 PM, Petr Stehlik <pstehlik@sophics.cz> wrote:
> >> ARAnyM hackers: What's the maximum size of ID names?
> >
> > There isn't any given limit AFAIK, it's based on a common sense when
> > creating a new NatFeat name.
> 
> Understood :-) What's the size of the longest name in use?

aranym/src/natfeat$ grep name\(\) * | grep return
debugprintf.h:	const char *name() { return "DEBUGPRINTF"; }
ethernet.h:	const char *name() { return "ETHERNET"; }
hostfs.h:	const char *name() { return "HOSTFS"; }
nfaudio.h:	const char *name() { return "AUDIO"; }
nf_basicset.h:	const char *name() { return "NF_NAME"; }
nf_basicset.h:	const char *name() { return "NF_VERSION"; }
nf_basicset.h:	const char *name() { return "NF_SHUTDOWN"; }
nf_basicset.h:	const char *name() { return "NF_STDERR"; }
nfbootstrap.h:	const char *name() { return "BOOTSTRAP"; }
nfcdrom.h:	const char *name() { return "CDROM"; }
nfclipbrd.h:	const char *name() { return "CLIPBRD"; }
nfjpeg.h:	const char *name() { return "JPEG"; }
nfosmesa.h:	const char *name() { return "OSMESA"; }
nfpci.h:	const char *name() { return "PCI"; }
nfvdi.h:	const char *name() { return "fVDI"; }
usbhost.h:	const char *name() { return "USBHOST"; }
xhdi.h:		const char *name() { return "XHDI"; }

> Is 32 OK?

yes, for now :)

Petr

Re: [PATCH] make NatFeat drivers bool, not tristate

[Thorsten Glaser]

Geert Uytterhoeven dixit:

>The patch below adds a wrapper that copies the (supposedly short) name to
>the kernel stack before calling into ARAnyM.

Is that enough? What about buffers and all that?

Sorry, won’t be testing these patches for a while,
I need to concentrate on the Debian sid kernel for now.

bye,
//mirabilos
-- 
<ch> you introduced a merge commit        │<mika> % g rebase -i HEAD^^
<mika> sorry, no idea and rebasing just fscked │<mika> Segmentation
<ch> should have cloned into a clean repo      │  fault (core dumped)
<ch> if I rebase that now, it's really ugh     │<mika:#grml> wuahhhhhh

Re: [PATCH] make NatFeat drivers bool, not tristate

[Geert Uytterhoeven]

On Wed, Jul 24, 2013 at 7:23 PM, Thorsten Glaser <tg@mirbsd.de> wrote:
>>The patch below adds a wrapper that copies the (supposedly short) name to
>>the kernel stack before calling into ARAnyM.
>
> Is that enough? What about buffers and all that?

As m68k doesn't support highmem, all memory buffers should point to
"normal" kernel virtual memory. If any of these were a problem, we should
have noticed already.

I had a quick look at our nf* drivers, and (unless I missed something) none
of them seem to pass pointers to static data to NF calls, except for the
ID names and the buffer in arch/m68k/emu/natfeat.c:nfprint().
The latter is OK (albeit not thread-safe, but no one calls nfprint()?),
as natfeat.c is never a module.

> Sorry, won’t be testing these patches for a while,
> I need to concentrate on the Debian sid kernel for now.

OK. Enjoy sid, thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] make NatFeat drivers bool, not tristate

[Geert Uytterhoeven]

On Wed, Jul 24, 2013 at 12:51 AM, Thorsten Glaser <tg@debian.org> wrote:
> - nfeth: says "No such device" upon insmod

You're lucky. When insmodding nfeth.ko, my ARAnyM just terminates with:

Gotcha! Illegal memory access. Atari PC = $968c
If the Full History was enabled you would see the last 20 instructions here.

With my patch, insmod works again, and the network works, too.
AFAIK you cannot have two physical memory blocks with ARAnyM, so using
a copy on the stack should be safe.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus TorvaldsIf the Full History
was enabled you would see the last 20 instructions here.

Re: [PATCH] make NatFeat drivers bool, not tristate

[Thorsten Glaser]

Geert Uytterhoeven dixit:

>On Wed, Jul 24, 2013 at 12:51 AM, Thorsten Glaser <tg@debian.org> wrote:
>> - nfeth: says "No such device" upon insmod
>
>You're lucky. When insmodding nfeth.ko, my ARAnyM just terminates with:
>
>Gotcha! Illegal memory access. Atari PC = $968c

That’s http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=698064

>With my patch, insmod works again, and the network works, too.

Nice!

>AFAIK you cannot have two physical memory blocks with ARAnyM, so using
>a copy on the stack should be safe.

Hmm, okay. I think the ARAnyM code also only checks that
beginning and end are physical memory, so it’s contiguous.

bye,
//mirabilos
-- 
FWIW, I'm quite impressed with mksh interactively. I thought it was much
*much* more bare bones. But it turns out it beats the living hell out of
ksh93 in that respect. I'd even consider it for my daily use if I hadn't
wasted half my life on my zsh setup. :-) -- Frank Terbeck in #!/bin/mksh