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 <[email protected]>
Software Engineer, Linux Technology Center
IBM Corporation, Austin, TX
Alan Cox <[email protected]> on 05/16/2001 07:11:34 PM
To: Paul Schroeder/Austin/IBM@IBMUS
cc: [email protected], 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
> 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