Re: [RFC PATCHv4 4/7] HSI: hsi_char: Add HSI char device driver

[Alan Cox]

> +#define HSI_CHST_RD(c)		((c)->state & HSI_CHST_RD_MASK)
> +#define HSI_CHST_WR(c)		((c)->state & HSI_CHST_WR_MASK)
> +
> +#define HSI_CHST_OC_SET(c, v) \
> +	do { \
> +		(c)->state &= ~HSI_CHST_OC_MASK; \
> +		(c)->state |= v; \
> +	} while (0);
> +
> +#define HSI_CHST_RD_SET(c, v) \
> +	do { \
> +		(c)->state &= ~HSI_CHST_RD_MASK; \
> +		(c)->state |= v; \
> +	} while (0);
> +
> +#define HSI_CHST_WR_SET(c, v) \
> +	do { \
> +		(c)->state &= ~HSI_CHST_WR_MASK; \
> +		(c)->state |= v; \
> +	} while (0);


These sort of macros just hide detail - eg the lack of internal locking,
which can lead to problems later, plus they reference their arguments
multiple times so may have weird side effects.

They should probably be inline functions so they at least type check and
behave sanely, and their locking rules defintiely need documenting



> +static long hsi_char_ioctl(struct file *file, unsigned int cmd,
> +							unsigned long arg)
> +{
> +	struct hsi_char_channel *channel = file->private_data;
> +	unsigned int state;
> +	struct hsi_config cfg;
> +	struct hsc_rx_config rx_cfg;
> +	struct hsc_tx_config tx_cfg;
> +	long ret = 0;
> +
> +	if (HSI_CHST_OC(channel) != HSI_CHST_OPENED)
> +		return -ENODEV;

-EIO is the traditional response in this case if the channel has been
closed by the other end - ENODEV indicates there is no physical device
present - not sure which is right here from the code.


> +		} else if ((state == HSC_PM_ENABLE)
> +				&& (channel->wlrefcnt > 0)) {
> +			ret = hsi_stop_tx(channel->cl);
> +			if (!ret)
> +				channel->wlrefcnt--;

What locks this lot against races (ditto some of the other ioctl code)

> +	refcnt = atomic_inc_return(&cl_data->refcnt);
> +	if (refcnt == 1) {

You seem to construct a lot of clever stuff using atomic_inc_return and
friends where it looks like test_and_set_bit - or even a mutex over
open/close would be far easier to understand ?

> +	ret = hsi_char_msgs_alloc(channel);
> +
> +	if (ret < 0) {
> +		refcnt = atomic_dec_return(&cl_data->refcnt);
> +		if (!refcnt)
> +			hsi_release_port(channel->cl);
> +		spin_lock_bh(&channel->lock);
> +		HSI_CHST_OC_SET(channel, HSI_CHST_CLOSED);
> +		goto out;
> +	}
> +	if (refcnt == 1)
> +		cl_data->attached = 1;

What happens here if a second open passes the first, the attached will
stay zero and other stuff will be in strange states but the open flag
will be set ?


> +	for (i = 0; i < HSI_CHAR_DEVS && channels_map[i] >= 0; i++) {
> +		if (channels_map[i] >= HSI_CHAR_DEVS) {
> +			pr_err("Invalid HSI/SSI channel specified");
> +			return -EINVAL;
> +		}
> +		set_bit(channels_map[i], &ch_mask);

How will this integrate with hot discovery of SSI supporting devices ?

--
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/