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@xxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
