DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0F06520835
Subject: Re: [PATCH 13/19] wilc: add linux_wlan.c
To:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
CC:     <kvalo@codeaurora.org>, <gregkh@linuxfoundation.org>,
        <ganesh.krishna@microchip.com>, <aditya.shankar@microchip.com>,
        <venkateswara.kaja@microchip.com>, <claudiu.beznea@microchip.com>,
        <adham.abozaeid@microchip.com>
References: <1537957525-11467-1-git-send-email-ajay.kathat@microchip.com>
 <1537957525-11467-14-git-send-email-ajay.kathat@microchip.com>
 <1539009674.3687.75.camel@sipsolutions.net>
From:   Ajay Singh <ajay.kathat@microchip.com>
Message-ID: <45c17836-9362-d2ef-9647-514b5c75ec93@microchip.com>
Date:   Thu, 11 Oct 2018 12:30:41 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <1539009674.3687.75.camel@sipsolutions.net>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Received-SPF: None (protection.outlook.com: microchip.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2018 07:01:01.9842 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3461676a-e5f4-40ee-8c51-08d62f4750c9
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB2567
X-OriginatorOrg: microchip.com
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org


On 10/8/2018 8:11 PM, Johannes Berg wrote:
> On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
>> Moved '/driver/staging/wilc1000/linux_wlan.c' to
>> 'drivers/net/wireless/microchip/wilc/'.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>> ---
>>  drivers/net/wireless/microchip/wilc/linux_wlan.c | 1161 ++++++++++++++++++++++
>>  1 file changed, 1161 insertions(+)
>>  create mode 100644 drivers/net/wireless/microchip/wilc/linux_wlan.c
> Hmm. It's pretty obviously a linux driver, what's the point?
>
Agree, will rename by avoiding the 'linux' prefix.
>> diff --git a/drivers/net/wireless/microchip/wilc/linux_wlan.c b/drivers/net/wireless/microchip/wilc/linux_wlan.c
>> new file mode 100644
>> index 0000000..76c9012
>> --- /dev/null
>> +++ b/drivers/net/wireless/microchip/wilc/linux_wlan.c
>> @@ -0,0 +1,1161 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2012 - 2018 Microchip Technology Inc., and its subsidiaries.
>> + * All rights reserved.
>> + */
>> +
>> +#include <linux/irq.h>
>> +#include <linux/kthread.h>
>> +#include <linux/firmware.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/inetdevice.h>
>> +
>> +#include "wilc_wfi_cfgoperations.h"
>> +
>> +static int dev_state_ev_handler(struct notifier_block *this,
>> +				unsigned long event, void *ptr)
>> +{
>> +	struct in_ifaddr *dev_iface = ptr;
>> +	struct wilc_priv *priv;
>> +	struct host_if_drv *hif_drv;
>> +	struct net_device *dev;
>> +	u8 *ip_addr_buf;
>> +	struct wilc_vif *vif;
>> +	u8 null_ip[4] = {0};
>> +	char wlan_dev_name[5] = "wlan0";
> Regardless of what you're trying to do, thta seems like a bad idea.
>
>> +	if (!dev_iface || !dev_iface->ifa_dev || !dev_iface->ifa_dev->dev)
>> +		return NOTIFY_DONE;
>> +
>> +	if (memcmp(dev_iface->ifa_label, "wlan0", 5) &&
>> +	    memcmp(dev_iface->ifa_label, "p2p0", 4))
>> +		return NOTIFY_DONE;
> That too. What???
Purpose of this function is to deferred going into powsersave till the
IP address is acquired by the interfaces. This is to avoid any issue in
acquiring the IP address(due to power save mode).
We will investigate and check the better to handle this condition.
>> +	dev  = (struct net_device *)dev_iface->ifa_dev->dev;
>> +	if (!dev->ieee80211_ptr || !dev->ieee80211_ptr->wiphy)
>> +		return NOTIFY_DONE;
>> +
>> +	priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
>> +	if (!priv)
>> +		return NOTIFY_DONE;
>> +
>> +	hif_drv = (struct host_if_drv *)priv->hif_drv;
>> +	vif = netdev_priv(dev);
>> +	if (!vif || !hif_drv)
>> +		return NOTIFY_DONE;
>> +
>> +	switch (event) {
>> +	case NETDEV_UP:
>> +		if (vif->iftype == STATION_MODE || vif->iftype == CLIENT_MODE) {
>> +			hif_drv->ifc_up = 1;
>> +			vif->obtaining_ip = false;
>> +			del_timer(&vif->during_ip_timer);
>> +		}
>> +
>> +		if (vif->wilc->enable_ps)
>> +			wilc_set_power_mgmt(vif, 1, 0);
>> +
>> +		netdev_dbg(dev, "[%s] Up IP\n", dev_iface->ifa_label);
>> +
>> +		ip_addr_buf = (char *)&dev_iface->ifa_address;
>> +		netdev_dbg(dev, "IP add=%d:%d:%d:%d\n",
>> +			   ip_addr_buf[0], ip_addr_buf[1],
>> +			   ip_addr_buf[2], ip_addr_buf[3]);
> %pI4, I believe, but I think you should just remove it, it likely won't
> have an IP address anyway, and you might have multiple, and so this is
> just broken.
>
>> +	eth_h = (struct ethhdr *)(skb->data);
>> +	if (eth_h->h_proto == cpu_to_be16(0x8e88))
>> +		netdev_dbg(ndev, "EAPOL transmitted\n");
> Err, no, just remove that.
Ok.
>> +	ih = (struct iphdr *)(skb->data + sizeof(struct ethhdr));
> Sure, everything is IP. You just checked that it wasn't EAPOL?
>
>> +	udp_buf = (char *)ih + sizeof(struct iphdr);
>> +	if ((udp_buf[1] == 68 && udp_buf[3] == 67) ||
>> +	    (udp_buf[1] == 67 && udp_buf[3] == 68))
>> +		netdev_dbg(ndev, "DHCP Message transmitted, type:%x %x %x\n",
>> +			   udp_buf[248], udp_buf[249], udp_buf[250]);
> Umm... no. Just remove that too.
>
Ok
>> +	vif->netstats.tx_packets++;
>> +	vif->netstats.tx_bytes += tx_data->size;
>> +	tx_data->bssid = wilc->vif[vif->idx]->bssid;
>> +	queue_count = wilc_wlan_txq_add_net_pkt(ndev, (void *)tx_data,
>> +						tx_data->buff, tx_data->size,
>> +						linux_wlan_tx_complete);
>> +
>> +	if (queue_count > FLOW_CONTROL_UPPER_THRESHOLD) {
>> +		netif_stop_queue(wilc->vif[0]->ndev);
>> +		netif_stop_queue(wilc->vif[1]->ndev);
>> +	}
> It seems like a pretty bad idea to hard-code two interfaces, we do
> dynamic addition/removal these days, in *particular* for P2P.
>
Did you mean it not good to call stop queue for both the interfaces.
Can you please provide some more details about this comments.
>> +static int wilc_mac_close(struct net_device *ndev)
>> +{
>> +	struct wilc_priv *priv;
>> +	struct wilc_vif *vif = netdev_priv(ndev);
>> +	struct host_if_drv *hif_drv;
>> +	struct wilc *wl;
>> +
>> +	if (!vif || !vif->ndev || !vif->ndev->ieee80211_ptr ||
>> +	    !vif->ndev->ieee80211_ptr->wiphy)
>> +		return 0;
> I'm not really sure why you're so paranoid, none of that can possibly
> happen.
>
>> +	priv = wiphy_priv(vif->ndev->ieee80211_ptr->wiphy);
>> +	wl = vif->wilc;
>> +
>> +	if (!priv)
>> +		return 0;
> Nor can this.
>
>> +	hif_drv = (struct host_if_drv *)priv->hif_drv;
>> +
>> +	netdev_dbg(ndev, "Mac close\n");
>> +
>> +	if (!wl)
>> +		return 0;
>> +
>> +	if (!hif_drv)
>> +		return 0;
> Nor these.

Sure, will remove the unnecessary check in this function.

Regards,
Ajay