RE: [PATCH net-next] net/usb: new driver for RTL8152

[hayeswang <hayeswang@realtek.com>]

 Oliver Neukum [mailto:oneukum@suse.de] 
> Sent: Friday, April 26, 2013 7:57 PM
> To: Hayeswang
> Cc: gregkh@linuxfoundation.org; netdev@vger.kernel.org; 
> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; nic_swsd
> Subject: Re: [PATCH net-next] net/usb: new driver for RTL8152
> 
[...]
> > +static inline void set_ethernet_addr(struct r8152 *tp)
> > +{
> > +	struct net_device *dev = tp->netdev;
> > +	u8 node_id[8] = {0};
> > +
> > +	if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) < 0)
> 
> DMA coherency rules. No buffers on the stack.

Excuse me. I don't understand what you mean. I reference the rtl8150.c and it
uses the same way. Besides, when I check the other drivers, I find the data
pointer of the parameter of the usb_control_msg() may be a pointer of a local
variable from the other functions.

[...]
> > +static void rtl_work_func_t(struct work_struct *work)
> > +{
> > +	struct r8152 *tp = container_of(work, struct r8152, 
> schedule.work);
> > +
> > +	if (!netif_running(tp->netdev))
> > +		goto out1;
> > +
> > +	if (test_bit(RTL8152_UNPLUG, &tp->flags))
> > +		goto out1;
> > +
> > +	set_carrier(tp);
> > +
> > +	if (test_bit(RTL8152_SET_RX_MODE, &tp->flags))
> > +		_rtl8152_set_rx_mode(tp->netdev);
> > +
> > +	schedule_delayed_work(&tp->schedule, HZ);
> 
> Why does this need to run permanently?

It is used to periodically check the speed, link status, and any other tasks
which need to be finished.

[...]
> > +static int rtl8152_close(struct net_device *netdev)
> > +{
> > +	struct r8152 *tp = netdev_priv(netdev);
> > +	int res = 0;
> > +
> > +	cancel_delayed_work_sync(&tp->schedule);
> 
> Looks like a race. What makes sure the work isn't rescheduled?

netif_running would be checked.

 
Best Regards,
Hayes