Re: [PATCH 08/40] wl12xx: wl12xx-fw-3 - update commands & events

[Luciano Coelho <coelho@ti.com>]

On Wed, 2011-08-10 at 12:53 +0300, Eliad Peller wrote: 
> hi Luca,
> 
> thanks for your (ongoing) reviews.
> i'll apply your comments in the next version.

Thanks.  It will still take some time for me to finish going through all
these 40 patches, as I don't want to starve my other workqueues. ;)


> On Wed, Aug 10, 2011 at 12:19 PM, Luciano Coelho <coelho@ti.com> wrote:
> > On Tue, 2011-08-09 at 12:13 +0300, Eliad Peller wrote:
> >>       /* unmask required mbox events  */
> >>       wl->event_mask = BSS_LOSE_EVENT_ID |
> >>               SCAN_COMPLETE_EVENT_ID |
> >>               PS_REPORT_EVENT_ID |
> >> -             JOIN_EVENT_COMPLETE_ID |
> >>               DISCONNECT_EVENT_COMPLETE_ID |
> >>               RSSI_SNR_TRIGGER_0_EVENT_ID |
> >>               PSPOLL_DELIVERY_FAILURE_EVENT_ID |
> >>               SOFT_GEMINI_SENSE_EVENT_ID |
> >>               PERIODIC_SCAN_REPORT_EVENT_ID |
> >> -             PERIODIC_SCAN_COMPLETE_EVENT_ID;
> >> +             PERIODIC_SCAN_COMPLETE_EVENT_ID |
> >> +             DUMMY_PACKET_EVENT_ID |
> >> +             PEER_REMOVE_COMPLETE_EVENT_ID |
> >> +             BA_SESSION_RX_CONSTRAINT_EVENT_ID |
> >> +             REMAIN_ON_CHANNEL_COMPLETE_EVENT_ID;
> >>
> >>       if (wl->bss_type == BSS_TYPE_AP_BSS)
> >> -             wl->event_mask |= STA_REMOVE_COMPLETE_EVENT_ID |
> >> -                               INACTIVE_STA_EVENT_ID |
> >> +             wl->event_mask |= INACTIVE_STA_EVENT_ID |
> >>                                 MAX_TX_RETRY_EVENT_ID;
> >
> > Do we really need to mask this stuff separately?
> >
> we thought of masking the needed events according to the active role.
> anyway, i since there is no overlapping, and in the future there will
> be multiple roles, i guess we can just unmask them all together.

Yes, that was my point.  I think we don't need to care about unmasking
different events for different roles, because the events shouldn't be
sent in the wrong roles.

Actually, I started wondering why do we need an event mask at all! Okay,
someone will say, for power-saving blahblah, but do we really have any
real use case where we can use this feature in a smart way?


> >> -int wl1271_cmd_join(struct wl1271 *wl, u8 bss_type)
> >> +int wl1271_cmd_role_enable(struct wl1271 *wl, u8 role_type, u8 *role_id)
> >
> > s/wl1271_cmd_role_enable/wl12xx_cmd_role_enable/
> >
> > Same thing for wl1271_cmd_role_disable and all the other functions whose
> > declaration changed.
> >
> > I was thinking that we could use the role_id directly from the wl struct
> > here, but then I changed my mind, because I think it's not good that the
> > cmd functions themselves change the wl struct.  At some point we need to
> > split the HW-related part of wl (phy) from the context-related elements
> 
> well, that exactly our plan. after applying the all the pending series
> we'll start splitting up wl into global and per-vif data, in order to
> support multiple vifs.

Of course, I don't really see many different other options here. ;)


> > (vif).  For most of the cases, if not all, the cmd functions only use
> > the HW-related elements.
> >
> every command that takes role_id as param is basically per-vif, so i
> guess you're wrong here :)

Right, but what I meant was *before* your patchset.  And also I meant
the elements that are needed to send the command itself (like wl->flags,
wl->cmd_box_addr), not the elements that will actually go into the cmd
parameters.  Anyway, what matters here is that we're aligned. :)


> >> +
> >> +       memcpy(cmd->mac_address, wl->mac_addr, ETH_ALEN);
> >
> > To keep aligned with my comment about phy vs. vif context, I think it
> > would be nicer to pass the MAC address as an argument to this function
> > as well.  When we implement multirole, we will need different MAC
> > addresses for each role, so we can't really use the value from wl.
> >
> you are right, but let's leave it for a later stage.
> (i have some patch that replaces all the wl->mac_addr to vif->addr)

Okay, when I wrote this I had actually not seen the other many commands
that take stuff from the wl struct.  Let's do it all together.


> again, thanks for your reviews.
> i'll just apply all the required changes instead of ACKing each one :)

Yeah, easier like that.

-- 
Cheers,
Luca.