Re: [PATCH v1.2.26] wan: new driver retina

[Krzysztof Halasa <khc@pm.waw.pl>]

"Matti Linnanvuori" <mattilinn@gmail.com> writes:

> I would not like to change a legacy driver much.

But this is a new driver, right? I guess the SIOCDEVPRIVATEs will go
at some point so the change can't be really avoided.

>>> +static int fepci_char_open(struct inode *inode, struct file *filp)
>>> +{
>>> +     unsigned int minor = MINOR(inode->i_rdev);
>>> +     if (unlikely(minor >= find_cnt || card_privates[minor].pci_dev == NULL))
>>> +             return -ENXIO;
>>> +     filp->f_op = &fepci_char_fops;
>>> +     if (unlikely(!try_module_get(THIS_MODULE)))
>>> +             return -EBUSY;
>>
>> That won't work race-free, use owner field.
>
> You mean the f_op assignment? I have removed that.

Actually I meant try_module_get() from within the driver, for example
your module may get unloaded while in this function, before this
try_module_get(), and that would be fatal. If you set the owner field
the reference count will be incremented by the caller first.

Though you may be right about the assignement as well.

>>> +static int fepci_stream_open_down(struct net_device *dev,
>>> +                               struct fepci_ch_private *fp)
>>> +{
>>> +     unsigned tx_pages, rx_pages, tx_order, rx_order;
>>> +     unsigned page_number;
>>> +     unsigned int i;
>>> +
>>> +     if (unlikely(fp->in_eth_mode)) {
>>> +             dev_warn(&dev->dev,
>>> +                      "Interface is in Ethernet mode, "
>>> +                      "cannot open stream interface\n");
>>> +             return -EBUSY;
>>> +     }

> Private net device ioctls are locked with rtnl_lock, so I see no
> race there.

Yes, I thought stream mode = character device but now I see it's
another flavour of network device(?), which is never up and exists
only for the ability to receive netdev ioctls (though it also uses
char dev ioctls).

Did I get this right?

Why not simply use a character device, with normal read, write etc?
-- 
Krzysztof Halasa