Jason,
On Wed, Sep 29, 2010 at 11:19:18AM -0700, jason wrote:
> Henry,
>
> Henry Ptasinski wrote:
> > On Mon, Sep 27, 2010 at 01:37:18PM -0700, Jason Cooper wrote:
> >> @@ -1847,7 +1858,12 @@ dhd_add_if(dhd_info_t *dhd, int ifidx, void *handle, char *name,
> >> ASSERT(dhd && (ifidx < DHD_MAX_IFS));
> >>
> >> ifp = dhd->iflist[ifidx];
> >> - if (!ifp && !(ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t)))) {
> >> + if (!ifp) {
> >> + DHD_ERROR(("%s: dhd->iflist[ifidx] null\n", __func__));
> >> + return -EINVAL;
> >> + }
> >> + ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
> >> + if (!ifp) {
> >> DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
> >> return -ENOMEM;
> >> }
> >
> > I think you changed the logic here from AND to OR. I believe this would
> > be more correct:
> >
> > ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
> > if (!(dhd->iflist[ifidx]) && (!ifp)) {
> > DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
> > return -ENOMEM;
> > }
> >
>
> I was attempting to remove the checkpatch.pl error with as little
> interpretation as possible. The current code executes the MALLOC() if and
> only if ifp != NULL. eg, if and only if dhd->iflist[ifidx] != NULL.
Ah, missed that. My attempted fix has a memory leak.
> Do you really want to bail only when _both_ are NULL? What if the MALLOC()
> failed? Or, what if the dhd->iflist[ifidx] was NULL?
>
Yes, it should bail only when both are null. A few lines later,
dhd->iflist[ifidx] gets set to ifp, so the net result of this code is to
initialize dhd->iflist[ifidx] if it's not initialized, and fail otherwise.
Here's take two. With this change applied, your patch series passes a quick
smoke test (scan, associate, ping).
- Henry
diff --git a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
index db8f7bf..86eee74 100644
--- a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
+++ b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
@@ -1859,13 +1859,11 @@ dhd_add_if(dhd_info_t *dhd, int ifidx, void *handle, char *name,
ifp = dhd->iflist[ifidx];
if (!ifp) {
- DHD_ERROR(("%s: dhd->iflist[ifidx] null\n", __func__));
- return -EINVAL;
- }
- ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
- if (!ifp) {
- DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
- return -ENOMEM;
+ ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
+ if (!ifp) {
+ DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
+ return -ENOMEM;
+ }
}
memset(ifp, 0, sizeof(dhd_if_t));
--
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
