Date: Thu, 12 Feb 2015 12:00:12 -0800 From: Jack Vogel <jfvogel@gmail.com> To: Sreekanth Rupavatharam <rupavath@juniper.net> Cc: "jfv@freebsd.org" <jfv@freebsd.org>, "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, hiren panchasara <hiren@strugglingcoder.info> Subject: Re: Double cleanup in igb_attach Message-ID: <CAFOYbc=ed3vrp00aQXQsTPGzerysmYk%2BYEBDzi5TG9ZtxHZJbg@mail.gmail.com> In-Reply-To: <D102367E.1B663%rupavath@juniper.net> References: <D0EC151C.1A7B1%rupavath@juniper.net> <20150127192814.GA63990@strugglingcoder.info> <D0ED2984.1A86C%rupavath@juniper.net> <CAFOYbcmBks6s448stkVjGsWxxNZdZEwX2zxtdUh5cDnmtKEA1w@mail.gmail.com> <26266AD2-4743-4A7B-A87D-F68E2E2425A0@juniper.net> <CAFOYbckAP6t0jkhtswi%2BRnFGtJtN7=feCw6_vneS%2BSEZn4o2Pg@mail.gmail.com> <BABD0D32-30E3-4521-8838-976FF77AE244@juniper.net> <CAFOYbcmPF70yf%2B7BmTdDrrfUtyPP7pkCx8_%2BQdZcfOSA_dAK1w@mail.gmail.com> <D102367E.1B663%rupavath@juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
I do not recall if I put that call in myself and, yes, it seems odd. It was
probably trying to clean up
some bad state a failed attach left things in.
If it is removed it should be thoroughly regression tested.
Jack
On Thu, Feb 12, 2015 at 10:56 AM, Sreekanth Rupavatharam <
rupavath@juniper.net> wrote:
> Hi Jack,
> Actually, looking at the code again, it seems to me that igb_detach is not
> supposed to be called from igb_attach at all. It causes more problems than
> previously mentioned. E.g., in case of branching to err_late *before*
> igb_setup_interface(where the ifp is initialized), calling igb_detach there
> causes a NULL pointer access. The best way to deal with it is to take out
> the igb_detach call from the err_late: label. Thoughts? Comments?
> -- Thanks,
>
> Sreekanth
>
>
>
> From: Jack Vogel <jfvogel@gmail.com>
> Date: Tuesday, January 27, 2015 at 12:42 PM
> To: Sreekanth Rupavatharam <rupavath@juniper.net>
> Cc: hiren panchasara <hiren@strugglingcoder.info>, "
> freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, "jfv@freebsd.org" <
> jfv@freebsd.org>
> Subject: Re: Double cleanup in igb_attach
>
> Yes, I will look them over.
>
> Jack
>
>
> On Tue, Jan 27, 2015 at 12:38 PM, Sreekanth Rupavatharam <
> rupavath@juniper.net> wrote:
>
>> Thanks jack,
>> Now, can you please review these changes? And commit if you deem it
>> fit?
>>
>> Thanks,
>>
>> -Sreekanth
>>
>> On Jan 27, 2015, at 12:24 PM, "Jack Vogel" <jfvogel@gmail.com> wrote:
>>
>> Errrr, I am one of those people :) (jack.vogel@intel.com)
>>
>> Jack
>>
>>
>> On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam <
>> rupavath@juniper.net> wrote:
>>
>>> Definitely, but I didn't have the contact info of those people.
>>>
>>> Thanks,
>>>
>>> -Sreekanth
>>>
>>> On Jan 27, 2015, at 12:15 PM, "Jack Vogel" <jfvogel@gmail.com> wrote:
>>>
>>> If you want something committed to an Intel driver, asking Intel
>>> might be the
>>> courteous thing to do, don't you think?
>>>
>>> Jack
>>>
>>>
>>> On Tue, Jan 27, 2015 at 11:51 AM, Sreekanth Rupavatharam <
>>> rupavath@juniper.net> wrote:
>>>
>>>> Hiren,
>>>> Can you help commit this?
>>>>
>>>> Index: if_igb.c
>>>>
>>>> ===================================================================
>>>>
>>>> --- if_igb.c(revision 298053)
>>>>
>>>> +++ if_igb.c(working copy)
>>>>
>>>> @@ -723,7 +723,8 @@ igb_attach(device_t dev)
>>>>
>>>> return (0);
>>>>
>>>>
>>>>
>>>> err_late:
>>>>
>>>> -igb_detach(dev);
>>>>
>>>> +if(igb_detach(dev) == 0) /* igb_detach did the cleanup */
>>>>
>>>> +return(error);
>>>>
>>>> igb_free_transmit_structures(adapter);
>>>>
>>>> igb_free_receive_structures(adapter);
>>>>
>>>> igb_release_hw_control(adapter);
>>>>
>>>> -- Thanks,
>>>>
>>>> Sreekanth
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 1/27/15, 11:28 AM, "hiren panchasara" <hiren@strugglingcoder.info>
>>>> wrote:
>>>>
>>>> + Jack
>>>> On Tue, Jan 27, 2015 at 12:00:19AM +0000, Sreekanth Rupavatharam wrote:
>>>>
>>>> Apologies if this is not the right forum. In igb_attach function, we
>>>> have this code.
>>>> err_late:
>>>> igb_detach(dev);
>>>> igb_free_transmit_structures(adapter);
>>>> igb_free_receive_structures(adapter);
>>>> igb_release_hw_control(adapter);
>>>> err_pci:
>>>> igb_free_pci_resources(adapter);
>>>> if (adapter->ifp != NULL)
>>>> if_free(adapter->ifp);
>>>> free(adapter->mta, M_DEVBUF);
>>>> IGB_CORE_LOCK_DESTROY(adapter);
>>>> The problem is that igb_detach also does the same cleanup in it?s
>>>> body. Only exception is this case where it just returns EBUSY
>>>> /* Make sure VLANS are not using driver */
>>>> if (if_vlantrunkinuse(ifp)) {
>>>> device_printf(dev,"Vlan in use, detach first\n");
>>>> return (EBUSY);
>>>> }
>>>> I think the code in igb_attach should be changed to free up resources
>>>> only if the igb_detach returns an error. Here?s the patch for it.
>>>> Index: if_igb.c
>>>> ===================================================================
>>>> --- if_igb.c (revision 298025)
>>>> +++ if_igb.c (working copy)
>>>> @@ -723,7 +723,8 @@ igb_attach(device_t dev)
>>>> return (0);
>>>> err_late:
>>>> - igb_detach(dev);
>>>> + if(igb_detach(dev) == 0) /* igb_detach did the cleanup */
>>>> + return;
>>>> igb_free_transmit_structures(adapter);
>>>> Can anyone comment on it and tell me if my understanding is
>>>> incorrect?
>>>>
>>>>
>>>> Seems reasonable to me at the first glance.
>>>>
>>>> We need to call IGB_CORE_LOCK_DESTROY(adapter) before returning
>>>> though.
>>>>
>>>> cheers,
>>>> Hiren
>>>>
>>>>
>>>
>>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFOYbc=ed3vrp00aQXQsTPGzerysmYk%2BYEBDzi5TG9ZtxHZJbg>