Re: [PATCH 1/5] drivers/misc: Support for RF interface device framework

[Akhil Goyal <akhil.goyal@freescale.com>]

On 6/18/2013 2:58 AM, Arnd Bergmann wrote:
> On Monday 17 June 2013, akhil.goyal@freescale.com wrote:
>> +
>> +menuconfig RFDEVICES
>> +	default n
>> +	bool "RF interface device support"
>> +	---help---
>> +	Support for RF interface devices.
>> +	In a baseband system, different radios (RF PHYs) are
>> +	connected depending on required radio technology. Higher layer
>> +	stacks need to configure the radio according to required network mode.
>> +	Adding this support will export different radios connected in system
>> +	(in case of multi mode system)as RF interface deivces 'rf0', 'rf1' etc.
>> +	Higher layer stacks (running in user space)can use rfX device to
>> +	talk to a specific radio.
>> +
>> +	radio interface controller driver (Antenna controller) and RF PHY driver
>> +	connected to system must also be chosen.
>
> You should spell out "RF" at least once here, not everybody is familiar
> with the term.
Ok I will do that
>
>> +if RFDEVICES
>> +
>> +config FSL_AIC
>> +	default y
>> +	bool "Freescale Antenna Interface Controller (AIC)"
>> +	---help---
>> +	Freescale AIC controller (Antenna Interface Controller) is found
>> +	in bsc913x family of SOCs. AIC has six RF lanes and maximum four
>> +	RF PHYs can be connected and operated simultaneously.
>> +
>> +config ADI9361
>> +	default y
>> +	bool "ADI 9361 RF PHY"
>> +	---help---
>> +	ADI9361 RF phy driver.
>> +
>> +endif
>
> This should probably be part of a later patch.
Ok I will do that
>> +
>> +static struct rf_priv *rf_priv;
>> +static int rf_change_state(struct rf_ctrl_dev *rf_dev, unsigned int state);
>> +static int rf_attach_phy(struct rf_ctrl_dev *rf_dev, struct rf_phy_dev *phy);
>> +static int rf_open(struct inode *inode, struct file *filep);
>> +static int rf_release(struct inode *inode, struct file *filep);
>> +static ssize_t rf_read(struct file *, char __user *, size_t, loff_t *);
>> +static long rf_ioctl(struct file *, unsigned int, unsigned long);
>
> Please reorganize the file so that you don't need forward declarations for
> functions.
>
ok I will do that.
>> +struct rf_ctrl_dev *allocate_rf_ctrl_dev(size_t priv_size,
>> +		unsigned long flags)
>> +{
>> +	struct rf_ctrl_dev *rf_dev;
>> +	size_t size;
>> +
>> +	size = sizeof(struct rf_ctrl_dev) + priv_size;
>> +	rf_dev = kzalloc(size, flags);
>> +
>> +	if (!rf_dev)
>> +		return rf_dev;
>> +
>> +	atomic_set(&rf_dev->ref, 1);
>> +	mutex_init(&rf_dev->lock);
>> +	init_waitqueue_head(&rf_dev->wait_q);
>> +	INIT_LIST_HEAD(&rf_dev->event_handler_list);
>> +	spin_lock_init(&rf_dev->event_handler_lock);
>> +	raw_spin_lock_init(&rf_dev->wait_q_lock);
>> +	rf_dev->priv = (unsigned char *) rf_dev + sizeof(struct rf_ctrl_dev);
>> +	rf_dev->dev_idx = INVAL_DEV_IDX;
>> +
>> +	return rf_dev;
>> +}
>> +EXPORT_SYMBOL(allocate_rf_ctrl_dev);
>
> Normally I would expect to see EXPORT_SYMBOL_GPL.
ok i will correct this
>
>
>> +	/*
>> +	 * Spin_locks are changed to mutexes if PREEMPT_RT is enabled,
>> +	 * i.e they can sleep. This fact is problem for us because
>> +	 * add_wait_queue()/wake_up_all() takes wait queue spin lock.
>> +	 * Since spin lock can sleep with PREEMPT_RT, wake_up_all() can not be
>> +	 * called from rf_notify_dl_tti (which is called in interrupt context).
>> +	 * As a workaround, wait_q_lock is used for protecting the wait_q and
>> +	 * add_wait_queue_locked()/ wake_up_locked() functions of wait queues
>> +	 * are used.
>> +	 */
>> +	raw_spin_lock_irqsave(&rf_dev->wait_q_lock, flags);
>> +	__add_wait_queue_tail_exclusive(&rf_dev->wait_q,&wait);
>> +	raw_spin_unlock_irqrestore(&rf_dev->wait_q_lock, flags);
>> +	set_current_state(TASK_INTERRUPTIBLE);
>> +	/*Now wait here, tti notificaion will wake us up*/
>> +	schedule();
>> +	set_current_state(TASK_RUNNING);
>> +	raw_spin_lock_irqsave(&rf_dev->wait_q_lock, flags);
>> +	__remove_wait_queue(&rf_dev->wait_q,&wait);
>> +	raw_spin_unlock_irqrestore(&rf_dev->wait_q_lock, flags);
>
> This is not a proper method of waiting for an event. Why can't you
> use wait_event() here?
wait_event() is internally calling spin_lock_irqsave() and this function 
will be called in hard IRQ context with PREEMPT_RT enabled(IRQF_NODELAY 
set). So wait_event cannot be used.
This problem can be solved if we can get the following patch applied on 
the tree.
https://patchwork.kernel.org/patch/2161261/

>
> The explanation about the interrupt handler seems incorrect, since PREEMPT_RT
> also turns interrupt handlers into threads.
The interrupt handler has real time requirement and thus running in 
HARDIRQ context with flag IRQF_NODELAY. We get this interrupt in every 
millisecond.
>
>> +static long rf_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct rf_ctrl_dev *rf_dev;
>> +	struct rf_phy_dev *phy;
>> +	struct rf_init_params init_params;
>> +	struct rif_phy_cmd_set cmd_set;
>> +	struct rif_reg_buf reg_buf;
>> +	struct rif_write_reg_buf write_reg_buf;
>> +	struct rif_dac_params dac_params;
>> +	struct rif_dac_buf dac_buff;
>> +	struct rf_dev_info dev_info;
>> +	struct rf_tx_buf tx_buf;
>> +	struct rf_tx_en_dis tx_en_dis;
>> +	struct rf_rssi rssi;
>> +	struct rf_rx_gain rx_gain;
>> +	struct rf_gain_ctrl gain_ctrl;
>> +	struct rf_sniff_params sniff_params;
>> +	struct rf_synth_table synth_table;
>> +	struct rf_channel_params chan_params;
>> +	struct rf_event_listener listener;
>> +	unsigned long long (*params_buf)[NUM_SYNTH_PARAMS];
>> +	u8 (*reg_vals_buf)[NUM_SYNTH_REGS];
>
> Are you sure you don't run out of stack space here?
> It may also be beter for style reasons to split this function into a set
> of functions, one for each case.
ok I will split this function.
>
>> +	u32	*buf;
>> +	u32 u32arg;
>> +	int rc = -ENOSYS, size;
>> +
>> +	rf_dev = filep->private_data;
>> +	phy = rf_dev->phy;
>> +
>> +	rc = mutex_lock_interruptible(&rf_dev->lock);
>> +	switch (cmd) {
>
> If mutex_lock_interruptible() fails, you should not enter the
> function because you don't actually hold the mutex.
ok I will do that
>
>> +struct rif_phy_cmd {
>> +	__u32	param1;
>> +	__u32	param2;
>> +	__u32	param3;
>> +	__u8	cmd;
>> +};
>> +
>> +struct rif_phy_cmd_set {
>> +	struct rif_phy_cmd *cmds;
>> +	__u32 count;
>> +};
>
> Please try to avoid ioctl structures with pointers in them, they are harder
> to parse from anything that needs to intercept the ioctl in user space, and
> they break 64 bit compatibility mode.
ok I will do that
>
> You should also add padding in the first structure to ensure that the size
> is a multiple of the largest member. Otherwise you break e.g. ARM OABI
> support or anything else that has unconventional rules on the ABI.
ok i will do that
>
>> +struct time_sync_1588_cnt {
>> +	__u32 high;
>> +	__u32 low;
>> +};
>> +
>> +struct time_info {
>> +	struct time_sync_1588_cnt time_cnt;
>> +	__u8 event;
>> +};
>
> Here too.
>
>> +struct time_sync_data {
>> +	struct time_info time_info;
>> +	__u32 lte_delay;
>> +	__u32 ioctl_cmd;
>> +	__u32 black_out_duration;
>> +	__u8 correction_mode;
>> +	__u8 sync_source;
>> +};
>
> It definitely sounds like a bug to have a member named "ioctl_cmd" in
> a structure that is passed in an ioctl command.
Ok I will correct it.
>
>> +struct rif_dac_buf {
>> +	__u32 correction_type;
>> +	__u32 *buf;
>> +};
>
> A pointer again.
ok i will correct it.
>
>> +#define RF_MAGIC 0xEE
>> +#define RIF_DEV_INIT		_IOWR(RF_MAGIC, 1, struct rf_init_params)
>> +#define RIF_SET_TIMER_SOURCE	_IOW(RF_MAGIC, 2, unsigned int)
>> +#define RIF_GET_STATE		_IOR(RF_MAGIC, 3, unsigned int)
>> +#define RIF_SET_TIMER_CORRECTION _IOW(RF_MAGIC, 4, struct rif_dac_params)
>> +#define	RIF_RUN_PHY_CMDS	_IOW(RF_MAGIC, 5, struct rif_phy_cmd_set)
>> +#define RIF_READ_RSSI		_IOWR(RF_MAGIC, 6, struct rf_rssi)
>> +#define RIF_READ_PHY_REGS	_IOR(RF_MAGIC, 7, struct rif_reg_buf)
>> +#define RIF_READ_CTRL_REGS	_IOR(RF_MAGIC, 8, struct rif_reg_buf)
>> +#define RIF_START		_IO(RF_MAGIC, 9)
>> +#define RIF_STOP		_IO(RF_MAGIC, 10)
>> +#define RIF_GET_DEV_INFO	_IOWR(RF_MAGIC, 11, struct rf_dev_info)
>> +#define RIF_WRITE_PHY_REGS _IOR(RF_MAGIC, 12, struct rif_write_reg_buf)
>> +#define RIF_GET_DAC_VALUE	_IOR(RF_MAGIC, 13, struct rif_dac_buf)
>> +#define RIF_SET_TX_ATTEN	_IOW(RF_MAGIC, 14, struct rf_tx_buf)
>> +#define RIF_EN_DIS_TX		_IOW(RF_MAGIC, 15, struct rf_tx_en_dis)
>> +#define RIF_WRITE_CTRL_REGS 	_IOW(RF_MAGIC, 16, struct rif_write_reg_buf)
>> +#define RIF_READ_RX_GAIN 	_IOWR(RF_MAGIC, 17, struct rf_rx_gain)
>> +#define RIF_CONFIG_SNIFF 	_IOWR(RF_MAGIC, 18, struct rf_sniff_params)
>> +#define RIF_WRITE_RX_GAIN 	_IOW(RF_MAGIC, 19, struct rf_rx_gain)
>> +#define RIF_SET_GAIN_CTRL_MODE 	_IOW(RF_MAGIC, 20, struct rf_gain_ctrl)
>> +#define RIF_INIT_SYNTH_TABLE	_IOW(RF_MAGIC, 21, struct rf_synth_table)
>> +#define RIF_CHANNEL_OPEN	_IOW(RF_MAGIC, 22, struct rf_channel_params)
>> +#define RIF_CHANNEL_CLOSE	_IOW(RF_MAGIC, 23, unsigned int)
>> +#define RIF_REGISTER_EVENT	_IOW(RF_MAGIC, 24, struct rf_event_listener)
>> +#define RIF_UNREGISTER_EVENT	_IO(RF_MAGIC, 25)
>
> On the whole, the ioctl API looks very complex to me. It may well be that
> the complexity is necessary, but I cannot tell because I don't understand
> the subsystem. Can you find someone from another company that has hardware
> which would use the same subsystem, and have them do a review of the API
> to ensure it works for them as well?
>
Antenna controller is a Freescale designed hardware block in the BSC9131 
SOC. I am not aware of any other company which has similar kind of 
controller. I could not find any existing Antenna Controller Driver in 
kernel.

AD9361 RF PHY is being used by other companies also but again there is 
no open source driver for that.

This framework binds the Radio PHY and the Antenna Controller and expose 
the complete rf device(phy + controller) to the user space. Since I am 
not aware of other companies using similar hardware, I was hoping to get 
feedback about framework and APIs on the list itself.

Also I will try to explain more on the IOCTL APIs in the Documentation 
file that I have added in this patch.

-Akhil