Re: [PATCH] ACP Modem (Mwave)

Re: [PATCH] ACP Modem (Mwave)

[Paul Schroeder]

Okay..  I cleaned things up...  I was more careful about the globals this
time...

Also...  The driver now builds as mwave.o instead of mwavedd.o...  The
driver now registers it's UART as a serial device (Thomas Hood)..  So there
is no need to run setserial anymore...

Cheers...Paul...


---
Paul B Schroeder  <paulsch@us.ibm.com>
Software Engineer, Linux Technology Center
IBM Corporation, Austin, TX


Alan Cox <alan@lxorguk.ukuu.org.uk> on 07/12/2001 02:55:00 PM

To:   Paul Schroeder/Austin/IBM@IBMUS
cc:   linux-kernel@vger.kernel.org, Mike Sullivan/Austin/IBM@IBMUS,
      alan@lxorguk.ukuu.org.uk (Alan Cox)
Subject:  Re: [PATCH] ACP Modem (Mwave)



> The patch has been updated...  The updates primarily consist of Alan's
> suggested changes below... (thank you)  It applies against the 2.4.6
> kernel...

A quick glance through it:

dsp3780I_WriteDStore still touches user space with a spinlock held
(also doesnt check the get_user return)

The ioctl handlers do not check copy_from_user/to_user returns

IOCTL_MW_UNREGISTER_IPC will oops if fed bogus info (ipcnum should be
unsigned)

The return should be -ENOTTY not -ENOIOCTLCMD  unless its internal code
that catches NOIOCTLCMD and changes it before user space sees it

mwave_Read should be -EINVAL not -ENOSYS (ENOSYS means the entire read
syscall
in the OS isnt there)

In debug mode mwave_write accesses user space directly and may crash
(buf[0])

Trivial item - coding style uses foo(void) not foo() to indicate functions
taking no arguments

Still have globals like "dspio" "uartio"  "ClaimResources" etc

whats wrong with tp3780_uart_io etc for globals ?

Otherwise it looks close to ready

Alan

Re: [PATCH] ACP Modem (Mwave)

[paulsch]

Oh yeah...

It can be gotten here:

http://oss.software.ibm.com/acpmodem/

Directly:
http://oss.software.ibm.com/pub/acpmodem/mwave_linux-2.4.6.patch-20010718


On Wed, 18 Jul 2001, Paul Schroeder wrote:

> Okay..  I cleaned things up...  I was more careful about the globals this
> time...
>
> Also...  The driver now builds as mwave.o instead of mwavedd.o...  The
> driver now registers it's UART as a serial device (Thomas Hood)..  So there
> is no need to run setserial anymore...
>
> Cheers...Paul...
>
>
> ---
> Paul B Schroeder  <paulsch@us.ibm.com>
> Software Engineer, Linux Technology Center
> IBM Corporation, Austin, TX
>
>
> Alan Cox <alan@lxorguk.ukuu.org.uk> on 07/12/2001 02:55:00 PM
>
> To:   Paul Schroeder/Austin/IBM@IBMUS
> cc:   linux-kernel@vger.kernel.org, Mike Sullivan/Austin/IBM@IBMUS,
>       alan@lxorguk.ukuu.org.uk (Alan Cox)
> Subject:  Re: [PATCH] ACP Modem (Mwave)
>
>
>
> > The patch has been updated...  The updates primarily consist of Alan's
> > suggested changes below... (thank you)  It applies against the 2.4.6
> > kernel...
>
> A quick glance through it:
>
> dsp3780I_WriteDStore still touches user space with a spinlock held
> (also doesnt check the get_user return)
>
> The ioctl handlers do not check copy_from_user/to_user returns
>
> IOCTL_MW_UNREGISTER_IPC will oops if fed bogus info (ipcnum should be
> unsigned)
>
> The return should be -ENOTTY not -ENOIOCTLCMD  unless its internal code
> that catches NOIOCTLCMD and changes it before user space sees it
>
> mwave_Read should be -EINVAL not -ENOSYS (ENOSYS means the entire read
> syscall
> in the OS isnt there)
>
> In debug mode mwave_write accesses user space directly and may crash
> (buf[0])
>
> Trivial item - coding style uses foo(void) not foo() to indicate functions
> taking no arguments
>
> Still have globals like "dspio" "uartio"  "ClaimResources" etc
>
> whats wrong with tp3780_uart_io etc for globals ?
>
> Otherwise it looks close to ready
>
> Alan
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

-- 

Paul B Schroeder
paulsch@haywired.net

Re: [PATCH] ACP Modem (Mwave)

[Thomas Hood]

FYI here's a data point.

I just tried this latest driver and it builds properly; it
correctly induces the serial driver to create /dev/tts/1
when mwave.o loads and to delete it when mwave.o unloads;
and it allows me to establish a connection.  No problems
encountered so far on:
   ThinkPad 600 51U (266 MHz Pentium II processor)
   linux-2.4.6-ac2 + Mwave driver patch 20010718

MWAVE DRIVER USERS!  Please note that the name of the module
has changed from mwavedd.o to mwave.o .  You may have to 
change your /etc/modules.conf.  E.g., I had to change the
line 
   alias char-major-10-219 mwavedd
to
   alias char-major-10-219 mwave

Although the driver is called "mwave", it does not support
all functions of all Mwave DSPs.  Currently it only supports
the 3780i Mwave DSP and only for the purpose of Hayes-
compatible modem implementation.  The 3780i DSP is found on
certain IBM ThinkPad laptop models, including the ThinkPad 600.

I would like to take this opportunity to thank Paul Schroeder
Mike Sullivan, and IBM for contributing this GPLed driver to
Linux.

--
Thomas Hood

paulsch@haywired.net wrote:
> 
> Oh yeah...
> 
> It can be gotten here:
> 
> http://oss.software.ibm.com/acpmodem/
> 
> Directly:
> http://oss.software.ibm.com/pub/acpmodem/mwave_linux-2.4.6.patch-20010718
> 
> On Wed, 18 Jul 2001, Paul Schroeder wrote:
> 
> > Okay..  I cleaned things up...  I was more careful about the globals this
> > time...
> >
> > Also...  The driver now builds as mwave.o instead of mwavedd.o...  The
> > driver now registers it's UART as a serial device (Thomas Hood)..  So there
> > is no need to run setserial anymore...
> >
> > Cheers...Paul...

Re: [PATCH] ACP Modem (Mwave)

[Robert Love]

I thought I would add
one more datapoint ...
kernel 2.4.6-ac5 +
mwave-patch-20010718 on
IBM ThinkPad 600 41U:
works fine thus far.

The userspace tools,
init script, etc also
work flawless on the
current RedHat Rawhide
(beta for 7.2).

[12:16:52]rml@icbm:~#
cat /proc/version 
Linux version 2.4.6-ac5
(rml@icbm) (gcc version
2.96 20000731 (Red Hat
Linux 7.1 2.96-94)) #1
Tue Jul 17 16:16:14 EDT
2001

I don't use modules (I
enabled them
specifically for this),
so I would like to see
this able to compile
into the kernel.  To
that effect, I think
this should be submitted
for inclusion into the
kernel proper.  It is
GPL now, and it is
apparently stable.
Perhaps Alan can pick it
up for the -ac tree?

Once its in the kernel
RedHat and others need
to be encouraged to
include packages in
their distribution :) --
i see SuSE does already.

Good job IBM.

-- 
Robert M. Love
rml at ufl.edu
rml at tech9.net

[INFO] NNTP access to linux-kernel for Sympatico subscribers

[Thomas Hood]

FYI I just stumbled upon a newsgroup on Sympatico's news server
(news1.sympatico.ca, but you should use the server assigned to
you) that is subscribed to linux-kernel.  (Sympatico is a major
Canadian ISP.)  The newsgroup is called "linux.kernel".

Thomas Hood

Re: [PATCH] ACP Modem (Mwave)

[Paul Schroeder]

The patch has been updated...  The updates primarily consist of Alan's
suggested changes below... (thank you)  It applies against the 2.4.6
kernel...

It can be gotten here:  http://oss.software.ibm.com/acpmodem/
Or directly:  http://oss.software.ibm.com/acpmodem/mwave_linux-2.4.6.patch

Comments, questions, suggestions are appreciated...

Cheers...Paul...


---
Paul B Schroeder  <paulsch@us.ibm.com>
Software Engineer, Linux Technology Center
IBM Corporation, Austin, TX


Alan Cox <alan@lxorguk.ukuu.org.uk> on 05/16/2001 07:11:34 PM

To:   Paul Schroeder/Austin/IBM@IBMUS
cc:   linux-kernel@vger.kernel.org, Mike Sullivan/Austin/IBM@IBMUS
Subject:  Re: [PATCH]  ACP Modem (Mwave)



> Please throw any comments, questions, suggestions, hard objects this
way...

First obvious comments

+    while (uCount-- != 0) {
+         unsigned short val_lo, val_hi;
+         cli();
+         val_lo = InWordDsp(DSP_MsaDataISLow);
+         val_hi = InWordDsp(DSP_MsaDataDSISHigh);
+         put_user(val_lo, pusBuffer++);
+         put_user(val_hi, pusBuffer++);
+         sti();
+


1.   Please use spinlocks not cli/sti as they will go away probably in 2.5

2.   You can't touch user space holding interrupts off as it can't handle
     page faults

+void PaceMsaAccess(unsigned short usDspBaseIO)
+{
+    schedule();
+    udelay(100);
+    schedule();
+}

If you are trying to be friendly then add

     if(current->need_resched)
          schedule()

just to be more efficient


+BOOLEAN dsp3780I_GetIPCSource(unsigned short usDspBaseIO,
+                    unsigned short *pusIPCSourc

s/BOOLEAN/int
s/TRUE/0
s/FALSE/-Eappropriateval

would be more in keeping. Not a bug by any means


The ioctl locking seems wrong. It doesnt look like the DSP accesses are
locked against one another and you can issue multiple ioctls in parallel in
different threads


If mwave_read/write do nothing then they should really be returning an
error
code. 0 is EOF, count on write is success.

+BOOLEAN smapi_init()

you want (void) or you get compiler warnings on some compiler revisions

+    PRINTK_1(TRACE_SMAPI, "smapi::smapi_init entry\n");
+
+    usSmapiID = CMOS_READ(0x7C);
+    usSmapiID |= (CMOS_READ(0x7D) << 8);

CMOS reads/writes must be done holding the lock against other cmos users

+int Initialize(THINKPAD_BD_DATA * pBDData)

Please dont have globals with names so generic


Hope those are useful

Alan

Re: [PATCH] ACP Modem (Mwave)

[Alan Cox]

> The patch has been updated...  The updates primarily consist of Alan's
> suggested changes below... (thank you)  It applies against the 2.4.6
> kernel...

A quick glance through it:

dsp3780I_WriteDStore still touches user space with a spinlock held
(also doesnt check the get_user return)

The ioctl handlers do not check copy_from_user/to_user returns

IOCTL_MW_UNREGISTER_IPC will oops if fed bogus info (ipcnum should be
unsigned)

The return should be -ENOTTY not -ENOIOCTLCMD  unless its internal code
that catches NOIOCTLCMD and changes it before user space sees it

mwave_Read should be -EINVAL not -ENOSYS (ENOSYS means the entire read syscall
in the OS isnt there)

In debug mode mwave_write accesses user space directly and may crash
(buf[0])

Trivial item - coding style uses foo(void) not foo() to indicate functions
taking no arguments

Still have globals like "dspio" "uartio"  "ClaimResources" etc

whats wrong with tp3780_uart_io etc for globals ?

Otherwise it looks close to ready

Alan

[PATCH] ACP Modem (Mwave)

[Paul Schroeder]

Subject says it all...

The patch is the driver portion for the Mwave applied against the 2.4.4
kernel proper..

It was a little big to send directly to the list..  So...  You'll be able
to pick it up at http://www.ibm.com/linux/ltc/ shortly..  Until it goes up
there, it will be available at http://www.haywired.net/MWAVE/...

Please throw any comments, questions, suggestions, hard objects this way...

Cheers...Paul...


---
Paul B Schroeder  <paulsch@us.ibm.com>
Software Engineer, Linux Technology Center
IBM Corporation, Austin, TX

Re: [PATCH] ACP Modem (Mwave)

[Alan Cox]

> Please throw any comments, questions, suggestions, hard objects this way...

First obvious comments

+	while (uCount-- != 0) {
+		unsigned short val_lo, val_hi;
+		cli();
+		val_lo = InWordDsp(DSP_MsaDataISLow);
+		val_hi = InWordDsp(DSP_MsaDataDSISHigh);
+		put_user(val_lo, pusBuffer++);
+		put_user(val_hi, pusBuffer++);
+		sti();
+


1.	Please use spinlocks not cli/sti as they will go away probably in 2.5

2.	You can't touch user space holding interrupts off as it can't handle
	page faults

+void PaceMsaAccess(unsigned short usDspBaseIO)
+{
+	schedule();
+	udelay(100);
+	schedule();
+}

If you are trying to be friendly then add

	if(current->need_resched)
		schedule()

just to be more efficient


+BOOLEAN dsp3780I_GetIPCSource(unsigned short usDspBaseIO,
+			      unsigned short *pusIPCSourc

s/BOOLEAN/int
s/TRUE/0
s/FALSE/-Eappropriateval

would be more in keeping. Not a bug by any means


The ioctl locking seems wrong. It doesnt look like the DSP accesses are
locked against one another and you can issue multiple ioctls in parallel in
different threads


If mwave_read/write do nothing then they should really be returning an error
code. 0 is EOF, count on write is success.

+BOOLEAN smapi_init()

you want (void) or you get compiler warnings on some compiler revisions

+	PRINTK_1(TRACE_SMAPI, "smapi::smapi_init entry\n");
+
+	usSmapiID = CMOS_READ(0x7C);
+	usSmapiID |= (CMOS_READ(0x7D) << 8);

CMOS reads/writes must be done holding the lock against other cmos users

+int Initialize(THINKPAD_BD_DATA * pBDData)

Please dont have globals with names so generic


Hope those are useful

Alan