Re: [22/22] Cyclades PC300 driver: omnibus patch from merge up to final version (patches 01 through 20 inclusive)
From: Jiri Slaby <hidden>
Date: 2012-01-30 10:14:05
Also in:
lkml
On 01/30/2012 04:01 AM, Andrea Shepard wrote:
quoted hunk ↗ jump to hunk
--- a/drivers/net/wan/Kconfig +++ b/drivers/net/wan/Kconfig@@ -205,10 +205,8 @@ config WANXL_BUILD_FIRMWARE config PC300 tristate "Cyclades-PC300 support (RS-232/V.35, X.21, T1/E1 boards)" - depends on HDLC && PCI && BROKEN + depends on HDLC && PCI
You should do this as the very last in the series. You break the build by the very first patch.
quoted hunk ↗ jump to hunk
--- a/drivers/net/wan/pc300.h +++ b/drivers/net/wan/pc300.h
...
quoted hunk ↗ jump to hunk
@@ -143,13 +150,28 @@ * Memory access functions/macros * * (required to support Alpha systems) * ***************************************/ -#define cpc_writeb(port,val) {writeb((u8)(val),(port)); mb();} -#define cpc_writew(port,val) {writew((ushort)(val),(port)); mb();} -#define cpc_writel(port,val) {writel((u32)(val),(port)); mb();} +#ifdef __KERNEL__
Why is this in a header from drivers/ ? Perhaps you should move the header if it is an interface. User does not need to see cpc_writeb et al. anyway. And u8 and others are not defined there.
quoted hunk ↗ jump to hunk
+#define cpc_writeb(port, val) {writeb((u8)(val), \ + (void __iomem *)(port)); mb(); } +#define cpc_writew(port, val) {writew((u16)(val), \ + (void __iomem *)(port)); mb(); } +#define cpc_writel(port, val) {writel((u32)(val), \ + (void __iomem *)(port)); mb(); } + +#define cpc_readb(port) readb((void __iomem *)(port)) +#define cpc_readw(port) readw((void __iomem *)(port)) +#define cpc_readl(port) readl((void __iomem *)(port)) -#define cpc_readb(port) readb(port) -#define cpc_readw(port) readw(port) -#define cpc_readl(port) readl(port) +#else /* __KERNEL__ */ +#define cpc_writeb(port, val) (*((u8 *)(port)) = (u8)(val)) +#define cpc_writew(port, val) (*((u16 *)(port)) = (u16)(val)) +#define cpc_writel(port, val) (*((u32 *)(port)) = (u32)(val)) + +#define cpc_readb(port) (*((u8 *)(port))) +#define cpc_readw(port) (*((u16 *)(port))) +#define cpc_readl(port) (*((u32 *)(port))) + +#endif /* __KERNEL__ */ /****** Data Structures *****************************************************/@@ -291,16 +349,31 @@ typedef struct pc300patterntst { u16 num_errors; } pc300patterntst_t; +#ifdef __KERNEL__ + typedef struct pc300dev { struct pc300ch *chan; u8 trace_on; u32 line_on; /* DCD(X.21, RSV) / sync(TE) change counters */ u32 line_off; +#ifdef __KERNEL__
Double ifdef?
char name[16];
- struct net_device *dev;
+ hdlc_device *hdlc;
+ struct net_device *netdev;
+
+ void *private;
+ struct sk_buff *tx_skb;
+
+ enum {
+ CPC_DMA_FULL,
+ CPC_DMA_ERROR,
+ TRANSMISSION_ACTIVE,
+ CHANNEL_CLOSED
+ } reason_stopped;
#ifdef CONFIG_PC300_MLPPP
void *cpc_tty; /* information to PC300 TTY driver */
#endif
+#endif /* __KERNEL__ */
}pc300dev_t;...
quoted hunk ↗ jump to hunk
@@ -346,6 +450,8 @@ typedef struct pc300chconf { u32 tslot_bitmap; /* bit[i]=1 => timeslot _i_ is active */ } pc300chconf_t; +#ifdef __KERNEL__ + typedef struct pc300ch { struct pc300 *card; int channel;@@ -365,8 +471,10 @@ typedef struct pc300 { spinlock_t card_lock; } pc300_t; +#endif /* __KERNEL__ */ + typedef struct pc300conf { - pc300hw_t hw; + struct pc300hw_user hw; pc300chconf_t conf; } pc300conf_t;@@ -430,7 +538,19 @@ enum pc300_loopback_cmds { #define PC300_TX_QUEUE_LEN 100 #define PC300_DEF_MTU 1600 -/* Function Prototypes */ -int cpc_open(struct net_device *dev); +#ifdef __KERNEL__ + +int cpc_open(struct net_device *); + +#ifdef CONFIG_PC300_MLPPP +void cpc_tty_init(pc300dev_t *); +void cpc_tty_unregister_service(pc300dev_t *); +void cpc_tty_receive(pc300dev_t *); +void cpc_tty_trigger_poll(pc300dev_t *); +void cpc_tty_reset_var(void); +#endif /* CONFIG_PC300_MLPP */ + +#endif /* __KERNEL__ */ #endif /* _PC300_H */ +
ETOOMANYIFDEFS. You should split the header. One part with the interface -> include/, the other with internal structures -> leave here.
quoted hunk ↗ jump to hunk
--- a/drivers/net/wan/pc300_drv.c +++ b/drivers/net/wan/pc300_drv.c@@ -1,6 +1,6 @@ #define USE_PCI_CLOCK static const char rcsid[] = -"Revision: 3.4.5 Date: 2002/03/07 "; +"Revision: 4.1.0 Date: 2004/02/20 ";
This is useless, isn't it? It doesn't mean anything after some time as kernel evolves.
quoted hunk ↗ jump to hunk
@@ -16,6 +16,13 @@ static const char rcsid[] = * 2 of the License, or (at your option) any later version. * * Using tabstop = 4. + * + * Cyclades version 4.1.0 merged in, with new portability fixes, + * and ported to recent kernels by Andrea Shepard <andrea@persephoneslair.org>
No, we have git log.
+ * Due to changes in certain ioctls necessary for portability, this + * version requires a new version of pc300utils, which may be found + * at: http://charon.persephoneslair.org/~andrea/software/pc300utils/ * * $Log: pc300_drv.c,v $ * Revision 3.23 2002/03/20 13:58:40 henrique
Another piece of cvs.
quoted hunk ↗ jump to hunk
@@ -238,21 +246,22 @@ static const char rcsid[] = #include "pc300.h" -#define CPC_LOCK(card,flags) \ - do { \ - spin_lock_irqsave(&card->card_lock, flags); \ - } while (0) +#define CPC_LOCK(card, flags) \ + { \ + spin_lock_irqsave(&((card)->card_lock), (flags)); \ + }
Nah. do-while has its meaning. Try this: if (1) CPC_LOCK(card,flags); else return; Overall, you don't need these macros at all.
-#define CPC_UNLOCK(card,flags) \
- do { \
- spin_unlock_irqrestore(&card->card_lock, flags); \
- } while (0)
+#define CPC_UNLOCK(card, flags) \
+ { \
+ spin_unlock_irqrestore(&((card)->card_lock), (flags)); \
+ }...
quoted hunk ↗ jump to hunk
@@ -279,53 +288,128 @@ MODULE_DEVICE_TABLE(pci, cpc_pci_dev_id);
...
-static void rx_dma_buf_check(pc300_t * card, int ch)
+static void rx_dma_buf_check(pc300_t *card, int ch)
{
- volatile pcsca_bd_t __iomem *ptdescr;
+ pcsca_bd_t __iomem *ptdescr;
int i;
u16 first_bd = card->chan[ch].rx_first_bd;
u16 last_bd = card->chan[ch].rx_last_bd;
int ch_factor;
ch_factor = ch * N_DMA_RX_BUF;
- printk("#CH%d: f_bd = %d, l_bd = %d\n", ch, first_bd, last_bd);
- for (i = 0, ptdescr = (card->hw.rambase +
- DMA_RX_BD_BASE + ch_factor * sizeof(pcsca_bd_t));
- i < N_DMA_RX_BUF; i++, ptdescr++) {
+ printk(KERN_DEBUG
+ "#CH%d: f_bd = %d, l_bd = %d\n", ch, first_bd, last_bd);
+ for (
+ i = 0,
+ ptdescr = (pcsca_bd_t *)
+ (card->hw.rambase + DMA_RX_BD_BASE +
+ ch_factor * sizeof(pcsca_bd_t));
+
+ i < N_DMA_RX_BUF;
+
+ i++,
+ ptdescr++
+ ) {This is horrible.
if (cpc_readb(&ptdescr->status) & DST_OSB)
- printk ("\n CH%d RX%d: next=0x%x, ptbuf=0x%x, ST=0x%x, len=%d",
- ch, i, cpc_readl(&ptdescr->next),
- cpc_readl(&ptdescr->ptbuf),
- cpc_readb(&ptdescr->status),
- cpc_readw(&ptdescr->len));
+ printk(KERN_DEBUG
+ "\n CH%d RX%d: next=0x%08x, ptbuf=0x%08x, ST=0x%2x, len=%d",
+ ch, i, (u32) cpc_readl(&ptdescr->next),
+ (u32) cpc_readl(&ptdescr->ptbuf),
+ cpc_readb(&ptdescr->status),
+ cpc_readw(&ptdescr->len));
}
- printk("\n");
+ printk(KERN_DEBUG "\n");
}...
-static void falc_intr_enable(pc300_t * card, int ch)
+static void falc_intr_enable(pc300_t *card, int ch)
{
pc300ch_t *chan = (pc300ch_t *) & card->chan[ch];
pc300chconf_t *conf = (pc300chconf_t *) & chan->conf;
falc_t *pfalc = (falc_t *) & chan->falc;
- void __iomem *falcbase = card->hw.falcbase;
+ uintptr_t falcbase = (uintptr_t)(card->hw.falcbase);Why is that?
quoted hunk ↗ jump to hunk
@@ -1940,18 +2212,19 @@ static void cpc_net_rx(struct net_device *dev) continue; } - dev->stats.rx_bytes += rxb; + stats->rx_bytes += rxb; #ifdef PC300_DEBUG_RX - printk("%s R:", dev->name); + printk(KERN_DEBUG "%s R:", dev->name); for (i = 0; i < skb->len; i++) - printk(" %02x", *(skb->data + i)); - printk("\n"); + printk(KERN_DEBUG " %02x", *(skb->data + i)); + printk(KERN_DEBUG "\n");
print_hex_dump_bytes();
quoted hunk ↗ jump to hunk
@@ -2486,73 +2847,157 @@ static void cpc_sca_status(pc300_t * card, int ch)
...
static int cpc_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
- pc300dev_t *d = (pc300dev_t *) dev_to_hdlc(dev)->priv;
+ pc300dev_t *d = (pc300dev_t *) (dev_to_hdlc(dev))->priv;
pc300ch_t *chan = (pc300ch_t *) d->chan;
pc300_t *card = (pc300_t *) chan->card;
pc300conf_t conf_aux;
pc300chconf_t *conf = (pc300chconf_t *) & chan->conf;
int ch = chan->channel;
- void __user *arg = ifr->ifr_data;
+ void *arg = (void *) ifr->ifr_data;But it is a userspace ptr!
quoted hunk ↗ jump to hunk
@@ -3361,47 +3897,67 @@ static void cpc_init_card(pc300_t * card)
...
- printk (" #%d, %dKB of RAM at 0x%08x, IRQ%d, channel %d.\n",
- board_nbr, card->hw.ramsize / 1024,
- card->hw.ramphys, card->hw.irq, i + 1);
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+ printk(KERN_INFO " #%d, %dKB of RAM at 0x%016lx, IRQ %d, channel %d.\n",
+ board_nbr, card->hw.ramsize / 1024,
+ (unsigned long)(card->hw.ramphys),
+ card->hw.irq, i + 1);
+#else /* !CONFIG_PHYS_ADDR_T_64BIT */
+ printk(KERN_INFO " #%d, %dKB of RAM at 0x%08x, IRQ %d, channel %d.\n",
+ board_nbr, card->hw.ramsize / 1024,
+ (unsigned int)(card->hw.ramphys),
+ card->hw.irq, i + 1);
+#endif /* CONFIG_PHYS_ADDR_T_64BIT */What about just a cast to ull?
quoted hunk ↗ jump to hunk
@@ -3495,29 +4081,63 @@ cpc_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
...
- card->hw.plxbase = ioremap(card->hw.plxphys, card->hw.plxsize); - card->hw.rambase = ioremap(card->hw.ramphys, card->hw.alloc_ramsize); - card->hw.scabase = ioremap(card->hw.scaphys, card->hw.scasize); + err = pci_enable_device(pdev); + if (err != 0) + goto err_release_sca; + + card->hw.plxbase = + (void __iomem *) ioremap(card->hw.plxphys, card->hw.plxsize);
Why do you need the cast? You may use pci_ioremap_bar anyway.
quoted hunk ↗ jump to hunk
--- a/drivers/net/wan/pc300_tty.c +++ b/drivers/net/wan/pc300_tty.c@@ -481,11 +502,35 @@ static int cpc_tty_write(struct tty_struct *tty, const unsigned char *buf, int c return -EINVAL; } - if (cpc_tty_send_to_card(cpc_tty->pc300dev, (void*)buf, count)) { - /* failed to send */ - CPC_TTY_DBG("%s: trasmition error\n", cpc_tty->name); - return 0; + if (from_user) {
Nowadays, it is always a kernel buffer. This check was removed years ago already I suppose. Please do not re-add crap from out of tree drivers.
+ unsigned char *buf_tmp;
+
+ buf_tmp = cpc_tty->buf_tx;
+ if (copy_from_user(buf_tmp, buf, count)) {
+ /* failed to copy from user */
+ CPC_TTY_DBG("%s: error in copy from user\n",
+ cpc_tty->name);
+ return -EINVAL;
+ }
+
+ if (cpc_tty_send_to_card(cpc_tty->pc300dev,
+ (void *)buf_tmp, count)) {
+ /* failed to send */
+ CPC_TTY_DBG("%s: transmission error\n", cpc_tty->name);
+ return 0;
+ }
+ } else {
+ if (
+ cpc_tty_send_to_card(cpc_tty->pc300dev,
+ (void *)buf,
+ count)
+ ) {
+ /* failed to send */
+ CPC_TTY_DBG("%s: transmission error\n", cpc_tty->name);
+ return 0;
+ }
}
+
return count;
}
...
quoted hunk ↗ jump to hunk
@@ -618,9 +666,18 @@ static void cpc_tty_flush_buffer(struct tty_struct *tty) CPC_TTY_DBG("%s: call wake_up_interruptible\n",cpc_tty->name); - tty_wakeup(tty); - return; -} + wake_up_interruptible(&tty->write_wait); + + if ( + (tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) && + tty->ldisc && + tty->ldisc->ops && tty->ldisc->ops->write_wakeup + ) { + CPC_TTY_DBG("%s: call line disc. wake up\n", + cpc_tty->name); + tty->ldisc->ops->write_wakeup(tty); + } +}
Another instance of such crap.
quoted hunk ↗ jump to hunk
@@ -665,35 +722,35 @@ static void cpc_tty_hangup(struct tty_struct *tty) */ static void cpc_tty_rx_work(struct work_struct *work) { - st_cpc_tty_area *cpc_tty; - unsigned long port; int i, j; - volatile st_cpc_rx_buf *buf; - char flags=0,flg_rx=1; - struct tty_ldisc *ld; + unsigned long port; + st_cpc_tty_area *cpc_tty; + st_cpc_rx_buf *buf; + char flags = 0, flg_rx = 1; if (cpc_tty_cnt == 0) return; - - for (i=0; (i < 4) && flg_rx ; i++) { - flg_rx = 0; + for (i = 0; (i < 4) && flg_rx; i++) { + flg_rx = 0; cpc_tty = container_of(work, st_cpc_tty_area, tty_rx_work); port = cpc_tty - cpc_tty_area; - for (j=0; j < CPC_TTY_NPORTS; j++) { + for (j = 0; j < CPC_TTY_NPORTS; j++) { cpc_tty = &cpc_tty_area[port]; - - if ((buf=cpc_tty->buf_rx.first) != NULL) { - if (cpc_tty->tty) { - ld = tty_ldisc_ref(cpc_tty->tty); - if (ld) { - if (ld->ops->receive_buf) { - CPC_TTY_DBG("%s: call line disc. receive_buf\n",cpc_tty->name); - ld->ops->receive_buf(cpc_tty->tty, (char *)(buf->data), &flags, buf->size); - } - tty_ldisc_deref(ld); - } - } + buf = cpc_tty->buf_rx.first; + if (buf != NULL) { + if (cpc_tty->tty && cpc_tty->tty->ldisc && + cpc_tty->tty->ldisc->ops && + cpc_tty->tty->ldisc->ops->receive_buf) {
Third instance of crap. NACK!
+ CPC_TTY_DBG( + "%s: call line disc. receive_buf\n", + cpc_tty->name); + cpc_tty->tty-> + ldisc->ops-> + receive_buf(cpc_tty->tty, + (char *)(buf->data), + &flags, buf->size); + } cpc_tty->buf_rx.first = cpc_tty->buf_rx.first->next; kfree((void *)buf); buf = cpc_tty->buf_rx.first;
...
quoted hunk ↗ jump to hunk
@@ -789,13 +848,13 @@ void cpc_tty_receive(pc300dev_t *pc300dev) } new = kmalloc(rx_len + sizeof(st_cpc_rx_buf), GFP_ATOMIC); - if (!new) { + if (new == 0) {
Nah.
quoted hunk ↗ jump to hunk
@@ -821,8 +880,7 @@ void cpc_tty_receive(pc300dev_t *pc300dev) cpc_tty->name); cpc_tty_rx_disc_frame(pc300chan); rx_len = 0; - kfree(new); - new = NULL; + kfree((unsigned char *)new);
Nah. Isn't the assignment necessary (I didn't check)?
quoted hunk ↗ jump to hunk
@@ -831,15 +889,14 @@ void cpc_tty_receive(pc300dev_t *pc300dev) cpc_tty_rx_disc_frame(pc300chan); stats->rx_dropped++; rx_len = 0; - kfree(new); - new = NULL; + kfree((unsigned char *)new);
Nah.
quoted hunk ↗ jump to hunk
@@ -892,15 +950,29 @@ static void cpc_tty_tx_work(struct work_struct *work) { st_cpc_tty_area *cpc_tty = container_of(work, st_cpc_tty_area, tty_tx_work); - struct tty_struct *tty; + struct tty_struct *tty; CPC_TTY_DBG("%s: cpc_tty_tx_work init\n",cpc_tty->name); - if ((tty = cpc_tty->tty) == NULL) { - CPC_TTY_DBG("%s: the interface is not opened\n",cpc_tty->name); + tty = cpc_tty->tty; + if (tty == 0) { + CPC_TTY_DBG("%s: the interface is not opened\n", + cpc_tty->name); return; } - tty_wakeup(tty); + + if ( + (tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) && + tty->ldisc && + tty->ldisc->ops && + tty->ldisc->ops->write_wakeup + ) { + CPC_TTY_DBG("%s:call line disc. wakeup\n", + cpc_tty->name); + tty->ldisc->ops->write_wakeup(tty); + } + + wake_up_interruptible(&tty->write_wait);
No!
quoted hunk ↗ jump to hunk
@@ -1032,38 +1108,40 @@ void cpc_tty_unregister_service(pc300dev_t *pc300dev) ulong flags; int res; - if ((cpc_tty= (st_cpc_tty_area *) pc300dev->cpc_tty) == NULL) { - CPC_TTY_DBG("%s: interface is not TTY\n", pc300dev->dev->name); + cpc_tty = (st_cpc_tty_area *) pc300dev->cpc_tty; + if (cpc_tty == 0) {
This is a pointer. 0 is *not* NULL in C.
if (--cpc_tty_cnt == 0) {
- if (serial_drv.refcount) {
- CPC_TTY_DBG("%s: unregister is not possible, refcount=%d",
- cpc_tty->name, serial_drv.refcount);
- cpc_tty_cnt++;
- cpc_tty_unreg_flag = 1;
- return;
- } else {
- CPC_TTY_DBG("%s: unregister the tty driver\n", cpc_tty->name);
- if ((res=tty_unregister_driver(&serial_drv))) {
- CPC_TTY_DBG("%s: ERROR ->unregister the tty driver error=%d\n",
- cpc_tty->name,res);
- }
+ CPC_TTY_DBG("%s: unregister the tty driver\n",
+ cpc_tty->name);
+ res = tty_unregister_driver(&serial_drv);
+ if (res) {
+ CPC_TTY_DBG(
+ "%s: ERROR ->unregister the tty driver error=%d\n",
+ cpc_tty->name, res);You may ignore the retval. As you could see, I NACK the series. Please fix the issues. thanks, -- js suse labs