Re: [RFC][PATCH 9/13] Equinox SST driver: tty interface

[Andrew Morton]

On Thu, 22 Jun 2006 09:20:28 -0400
"Straub, Michael" <Michael.Straub@xxxxxxxxxxx> wrote:

> Adds Equinox multi-port serial (SST) driver.
> 
> Part 9: new source file: drivers/char/eqnx/sst_tty.c.  Provides the
> standard
> TTY interface routines for the SST driver.
> 
> ...
>
> +/* defer call to write_wakeup */
> +int eqnx_write_wakeup_deferred = 0;

Please don't initilise static storage to zero - the C runtime environment
does that.  <entire patchset>

> +#define	MY_GROUP()	(process_group(current))

Remove this, open-code it at the callsite.

> +extern int SSTMINOR(unsigned int, unsigned int);
> +/**********************************************************************
> *****/
> +/* external variable and routines
> */
> +/**********************************************************************
> *****/
> +
> +extern int eqnx_nssps;
> +extern struct mpdev eqnx_dev[MAXSSP];
> +extern struct mpchan *eqnx_chan;
> +extern struct datascope eqnx_dscope[2];
> +extern char *eqnx_tmpwritebuf;
> +extern struct semaphore eqnx_tmpwritesem;
> +
> +extern void eqnx_megaparam(int);
> +extern int eqnx_modem(int, int);
> +extern void eqnx_frame_wait(struct mpchan *, int);
> +extern void eqnx_chnl_sync(struct mpchan *);
> +extern int SSTMINOR(unsigned int, unsigned int);
> +extern u32 eqnx_get_modem_info(struct mpchan *);
> +extern void eqnx_set_modem_info(struct mpchan *, unsigned int, unsigned
> int,
> +				struct tty_struct *);

In all patches, move all extern declarations to header files which are
shared by both the declaration site and by all users.

> +
> +		return (-ENODEV);

I'd second previous comments wrt overparenthesization.

> +	mpc->refcount++;

WHat's the locking for this field?

> +			mpc->openwaitcnt++;

Ditto.

> +		if (!page)
> +			return (-ENOMEM);

Please try to make all non-trivial functions have a single `return'
statement.  This driver like to sprinkle returns all over the place, and
that's an invitation to missed unlocks, resource leaks and duplicated
error-recovery code.  We use gotos for this.

> +	mpc->pgrp = MY_GROUP();

Why does this driver need to know about process groups?

> +void eqnx_close(struct tty_struct *tty, struct file *filp)
> +{
> +	struct mpchan *mpc;
> +	struct mpdev *mpd;
> +	int nchan, d;
> +	unsigned long flags;
> +	u16 attn;
> +
> +	/* channel validity checks */
> +	if (tty == (struct tty_struct *)NULL)
> +		return;
> +	mpc = (struct mpchan *)tty->driver_data;
> +	if (mpc == (struct mpchan *)NULL)
> +		return;

Please review the whole driver for unneeded typecasts of void*, remove them.

> +		(unsigned int)SSTMINOR(mpc->mpc_major, mpc->mpc_minor),
> count);

Remove SSTMINOR, open-code it.

> +/*
> + * delay_jiffies(len)
> + *
> + * Delay by the specified number of jiffies.
> + *
> + * len	= jiffies to delay.
> + */
> +static void delay_jiffies(int len)
> +{
> +	if (len > 0) {
> +		msleep(jiffies_to_msecs(len));
> +	}
> +}

It's unusual for a driver to want to sleep for a number of jiffies. 
Because HZ jiffies can vary by an order of magnitude.  msleep() is a better
interface to use.

This function is an obfuscated version of schedule_timeout_uninterruptible().

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/