Date:   Tue, 25 Jul 2017 17:50:38 +0300
From:   Aviad Krawczyk <aviad.krawczyk@...wei.com>
To:     Francois Romieu <romieu@...zoreil.com>
CC:     <davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>, <bc.y@...wei.com>,
        <victor.gissin@...wei.com>, <zhaochen6@...wei.com>,
        <tony.qu@...wei.com>
Subject: Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface

Hi,

hinic_remove - you are right, it is over safe code. We can remove it without
any crash. This case has never happened.

void * - I meant the problem is not in rq or sq, it can be in wq. But you confirmed
that it is ok to use union instead of the void* in wq. So, we will use union in wq.c
and in rq - rq_wqe and in sq - sq_wqe.

module_pci_driver - is not used in other drivers in the same segments, it is necessary?

Thanks,
Aviad


On 7/25/2017 2:03 AM, Francois Romieu wrote:
> Aviad Krawczyk <aviad.krawczyk@...wei.com> :
> [...]
>> hinic_remove - If insmod failed and someone calls rmmod, we will get a
>> crash because the resource are already free. Therefore we test if the
>> device exists, please tell me if you meant to something different
> 
> The module won't even proceed through the pci_driver remove method if
> the probe method failed. See drivers/pci/bus.c::pci_bus_add_device and
> track 'dev->is_added'. You don't need to believe me: try it.
> 
> I have no idea where your crash comes from but something does not
> look quite right.
> 
> (use module_pci_driver() to save some boilerplate code btw)
> 
> [...]
>> The priv data is in type void * because the
>> caller can use any struct that it wants, like the priv data in Linux
>> (netdev, irq, tasklet, work..) -
> 
> I disagree. A driver is a piece of glue between hardware and software.
> It fills a kernel's contract. It is not supposed to introduce opaque
> data (even if it's hard to resist).
> 
>> we can change it but if we will pass different struct
>> in the future, we will have to change the prototype of the functions.
> 
> It's fine. If I do something wrong - and at some point I will - I'd
> rather have it detected at compile time. Nobody wants to waste precious
> hardware lab testing time because of excess void *.
> 
>> According to the other void *:
>> The wq struct is used for cmdq, sq and rq. Therefore the wqe is in type
>> void *. There are 4 operations get_wqe, write_wqe, read_wqe and put_wqe - there
>> is no option that one function will be fed with a wrong pointer because the caller
>> should use what it got in get_wqe function.
>>
>> When something is used as multiple types, it can be used as void * or union.
>> Usually, I would prefer union. But, in this case if we will use union, maybe
>> there is a chance of using the wrong wqe type in the wrong work queue type.
> 
> union * will at least catch being fed a wrong type. void * won't notice.
> 
> Let's take a practical example: hinic_sq_get_sges.
> 
> void hinic_sq_get_sges(void *wqe, struct hinic_sge *sges, int nr_sges)
>                        ^^^^^^^^^
> {
>         struct hinic_sq_wqe *sq_wqe = (struct hinic_sq_wqe *)wqe;
> 
> 
> static void free_all_tx_skbs(struct hinic_txq *txq)
> {
>         struct hinic_dev *nic_dev = netdev_priv(txq->netdev);
>         struct hinic_sq *sq = txq->sq;
>         struct hinic_sq_wqe *wqe;
> [...]
>                 hinic_sq_get_sges(wqe, txq->free_sges, nr_sges);
> 
> 
> static int free_tx_poll(struct napi_struct *napi, int budget)
> {
> [...]
>         struct hinic_sq_wqe *wqe;
> [...]
>                 hinic_sq_get_sges(wqe, txq->free_sges, nr_sges);
> 
> 
> Why is it:
> 
> void hinic_sq_get_sges(void *wqe, ...
> 
> instead of:
> 
> void hinic_sq_get_sges(struct hinic_sq_wqe *wqe, ...
> 
> Because of a future change ?
> 

Powered by blists - more mailing lists