On 04/01/2010 01:18 AM, Ben Hutchings wrote:
> On Wed, 2010-03-31 at 21:42 +0200, L. Alberto Giménez wrote:
> [...]
>> --- /dev/null
>> +++ b/drivers/net/usb/ipheth.c
> [...]
Hi Ben,
Upstream has fixed several errors pointed out by you and Oliver (thanks
for that), but some of them are still pending.
I will send patches on top of my last driver submission (if the proper
way would be resubmit the whole code, please tell me. Anyway I need to
clarify some doubts...
>> + usb_fill_bulk_urb(dev->tx_urb, udev,
>> + usb_sndbulkpipe(udev, dev->bulk_out),
>> + dev->tx_buf, IPHETH_BUF_SIZE,
>> + ipheth_sndbulk_callback,
>> + dev);
>> + dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>> +
>> + retval = usb_submit_urb(dev->tx_urb, GFP_ATOMIC);
>> + if (retval) {
>> + err("%s: usb_submit_urb: %d", __func__, retval);
>> + dev->stats.tx_errors++;
>> + dev_kfree_skb_irq(skb);
>> + } else {
>> + net->trans_start = jiffies;
>
> No longer needed.
What is not longer needed? The assignment, the whole "else" branch? If
the assignment is what is not needed, can I just remove that line, right?
> [...]
>> +#ifdef HAVE_NET_DEVICE_OPS
>> +static const struct net_device_ops ipheth_netdev_ops = {
>> + .ndo_open = &ipheth_open,
>> + .ndo_stop = &ipheth_close,
>> + .ndo_start_xmit = &ipheth_tx,
>> + .ndo_tx_timeout = &ipheth_tx_timeout,
>> + .ndo_get_stats = &ipheth_stats,
>> +};
>> +#endif
>
> Remove the #ifdef, there is no question whether we have net_device_ops.
Ok, I will just remove both #ifdefs, but why is that? Maybe in previous
versions of the kernel the net_device_ops struct was introduced and now
it's present no matter how you configure your kernel?
> I have no idea about USB so I haven't checked the USB API usage at all.
I think that Greg is the maintainer for the USB subsystem, so if he has
no further commets, I will try to submit fixes for both your and
Oliver's comments along with the upstream developers.
Thanks for your comments.
Best regards,
--
L. Alberto Giménez
JabberID [email protected]
GnuPG key ID 0x3BAABDE1
On Fri, 2010-04-02 at 19:15 +0200, "L. Alberto Giménez" wrote:
> On 04/01/2010 01:18 AM, Ben Hutchings wrote:
> > On Wed, 2010-03-31 at 21:42 +0200, L. Alberto Giménez wrote:
> > [...]
> >> --- /dev/null
> >> +++ b/drivers/net/usb/ipheth.c
> > [...]
>
> Hi Ben,
>
> Upstream has fixed several errors pointed out by you and Oliver (thanks
> for that), but some of them are still pending.
>
> I will send patches on top of my last driver submission (if the proper
> way would be resubmit the whole code, please tell me. Anyway I need to
> clarify some doubts...
Since David Miller has not merged your original patch, you should send a
single new patch with the changes incorporated.
> >> + usb_fill_bulk_urb(dev->tx_urb, udev,
> >> + usb_sndbulkpipe(udev, dev->bulk_out),
> >> + dev->tx_buf, IPHETH_BUF_SIZE,
> >> + ipheth_sndbulk_callback,
> >> + dev);
> >> + dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> >> +
> >> + retval = usb_submit_urb(dev->tx_urb, GFP_ATOMIC);
> >> + if (retval) {
> >> + err("%s: usb_submit_urb: %d", __func__, retval);
> >> + dev->stats.tx_errors++;
> >> + dev_kfree_skb_irq(skb);
> >> + } else {
> >> + net->trans_start = jiffies;
> >
> > No longer needed.
>
> What is not longer needed? The assignment, the whole "else" branch? If
> the assignment is what is not needed, can I just remove that line, right?
The assignment is not needed.
> > [...]
> >> +#ifdef HAVE_NET_DEVICE_OPS
> >> +static const struct net_device_ops ipheth_netdev_ops = {
> >> + .ndo_open = &ipheth_open,
> >> + .ndo_stop = &ipheth_close,
> >> + .ndo_start_xmit = &ipheth_tx,
> >> + .ndo_tx_timeout = &ipheth_tx_timeout,
> >> + .ndo_get_stats = &ipheth_stats,
> >> +};
> >> +#endif
> >
> > Remove the #ifdef, there is no question whether we have net_device_ops.
>
> Ok, I will just remove both #ifdefs, but why is that? Maybe in previous
> versions of the kernel the net_device_ops struct was introduced and now
> it's present no matter how you configure your kernel?
[...]
Correct, it is now mandatory.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
On 04/02/2010 07:21 PM, Ben Hutchings wrote:
> On Fri, 2010-04-02 at 19:15 +0200, "L. Alberto Giménez" wrote:
>> On 04/01/2010 01:18 AM, Ben Hutchings wrote:
>>> On Wed, 2010-03-31 at 21:42 +0200, L. Alberto Giménez wrote:
>>> [...]
>>>> --- /dev/null
>>>> +++ b/drivers/net/usb/ipheth.c
>>> [...]
>>>> + usb_fill_bulk_urb(dev->tx_urb, udev,
>>>> + usb_sndbulkpipe(udev, dev->bulk_out),
>>>> + dev->tx_buf, IPHETH_BUF_SIZE,
>>>> + ipheth_sndbulk_callback,
>>>> + dev);
>>>> + dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>>>> +
>>>> + retval = usb_submit_urb(dev->tx_urb, GFP_ATOMIC);
>>>> + if (retval) {
>>>> + err("%s: usb_submit_urb: %d", __func__, retval);
>>>> + dev->stats.tx_errors++;
>>>> + dev_kfree_skb_irq(skb);
>>>> + } else {
>>>> + net->trans_start = jiffies;
>>> No longer needed.
>> What is not longer needed? The assignment, the whole "else" branch? If
>> the assignment is what is not needed, can I just remove that line, right?
>
> The assignment is not needed.
Hi,
I've been looking into this and it seems that the net_device.trans_start
field is now deprecated in favor of net_device.rx_queue.trans_start
-rx_queue is a struct netdev_queue- (file include/linux/netdevice.h), as
states the comment:
512 /*
513 * please use this field instead of dev->trans_start
514 */
515 unsigned long trans_start;
Reading LDD3 book, it says that the driver is reponsible for updating
trans_start (as well as trans_rx, but we're not talking about that one).
So, I guess that the LDD book is outdated (again ;) ), but what I don't
understand at all is wether the driver should keep updating the right
field (dev->rx_queue.trans_start) or if the fact that it's inside a
"queue" implies that the code that is responsible for that queue would
update the trans_start field by itself?
Thanks in advance.
Best regards,
--
L. Alberto Giménez
JabberID [email protected]
GnuPG key ID 0x3BAABDE1
On Fri, 2010-04-02 at 19:53 +0200, "L. Alberto Giménez" wrote:
> On 04/02/2010 07:21 PM, Ben Hutchings wrote:
> > On Fri, 2010-04-02 at 19:15 +0200, "L. Alberto Giménez" wrote:
> >> On 04/01/2010 01:18 AM, Ben Hutchings wrote:
> >>> On Wed, 2010-03-31 at 21:42 +0200, L. Alberto Giménez wrote:
> >>> [...]
> >>>> --- /dev/null
> >>>> +++ b/drivers/net/usb/ipheth.c
> >>> [...]
> >>>> + usb_fill_bulk_urb(dev->tx_urb, udev,
> >>>> + usb_sndbulkpipe(udev, dev->bulk_out),
> >>>> + dev->tx_buf, IPHETH_BUF_SIZE,
> >>>> + ipheth_sndbulk_callback,
> >>>> + dev);
> >>>> + dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> >>>> +
> >>>> + retval = usb_submit_urb(dev->tx_urb, GFP_ATOMIC);
> >>>> + if (retval) {
> >>>> + err("%s: usb_submit_urb: %d", __func__, retval);
> >>>> + dev->stats.tx_errors++;
> >>>> + dev_kfree_skb_irq(skb);
> >>>> + } else {
> >>>> + net->trans_start = jiffies;
> >>> No longer needed.
> >> What is not longer needed? The assignment, the whole "else" branch? If
> >> the assignment is what is not needed, can I just remove that line, right?
> >
> > The assignment is not needed.
>
> Hi,
>
> I've been looking into this and it seems that the net_device.trans_start
> field is now deprecated in favor of net_device.rx_queue.trans_start
[...]
The networking core updates netdev_queue::trans_start by calling
txq_trans_update() where necessary. Drivers should not touch either
net_device::trans_start or netdev_queue::trans_start any more.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.