netdev - Re: [PATCH net-next 6/6] ionic: useful names for booleans

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7163f799-d728-8672-9487-d69aa62754ae@pensando.io>
Date:   Thu, 5 Nov 2020 10:50:13 -0800
From:   Shannon Nelson <snelson@...sando.io>
To:     Saeed Mahameed <saeed@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org
Subject: Re: [PATCH net-next 6/6] ionic: useful names for booleans

On 11/4/20 5:21 PM, Saeed Mahameed wrote:
> On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
>> With a few more uses of true and false in function calls, we
>> need to give them some useful names so we can tell from the
>> calling point what we're doing.
>>
> Aha! The root cause of the issue is passing booleans to functions in
> first place, it is usually a bad idea that could lead to complication
> and overloading the design for no reason, please see my suggestion in
> the previous patch maybe you can apply the same approach on some of the
> booleans below.

Yes, this is similar to what I was commenting on when I responded to the 
in_interrupt() patches.  However, when the code around Add and Delete is 
identicle and called from multiple places, it is handy to put it in one 
place with a boolean so as to not have multiple instances to maintain.  
Yes, it's a tradeoff, and these #defines are meant to help ease the 
readability.

The v2 for patch 5/6 in this patchset might help this a little, I'll see 
what I can do.

Thanks for you time in looking through these patches.

Cheers,
sln


>
>> Signed-off-by: Shannon Nelson <snelson@...sando.io>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.c | 16 ++++++++-------
>> -
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.h |  8 ++++++++
>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index a58bb572b23b..a0d2989a0d8d 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -1074,22 +1074,22 @@ static int ionic_lif_addr(struct ionic_lif
>> *lif, const u8 *addr, bool add,
>>   
>>   static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, true, true);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR,
>> CAN_SLEEP);
>>   }
>>   
>>   static int ionic_ndo_addr_add(struct net_device *netdev, const u8
>> *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR,
>> CAN_NOT_SLEEP);
>>   }
>>   
>>   static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, false, true);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR,
>> CAN_SLEEP);
>>   }
>>   
>>   static int ionic_ndo_addr_del(struct net_device *netdev, const u8
>> *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR,
>> CAN_NOT_SLEEP);
>>   }
>>   
>>   static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int
>> rx_mode)
>> @@ -1214,7 +1214,7 @@ static void ionic_set_rx_mode(struct net_device
>> *netdev, bool from_ndo)
>>   
>>   static void ionic_ndo_set_rx_mode(struct net_device *netdev)
>>   {
>> -	ionic_set_rx_mode(netdev, true);
>> +	ionic_set_rx_mode(netdev, FROM_NDO);
>>   }
>>   
>>   static __le64 ionic_netdev_features_to_nic(netdev_features_t
>> features)
>> @@ -1805,7 +1805,7 @@ static int ionic_txrx_init(struct ionic_lif
>> *lif)
>>   	if (lif->netdev->features & NETIF_F_RXHASH)
>>   		ionic_lif_rss_init(lif);
>>   
>> -	ionic_set_rx_mode(lif->netdev, false);
>> +	ionic_set_rx_mode(lif->netdev, NOT_FROM_NDO);
>>   
>>   	return 0;
>>   
>> @@ -2813,7 +2813,7 @@ static int ionic_station_set(struct ionic_lif
>> *lif)
>>   		 */
>>   		if (!ether_addr_equal(ctx.comp.lif_getattr.mac,
>>   				      netdev->dev_addr))
>> -			ionic_lif_addr(lif, netdev->dev_addr, true,
>> true);
>> +			ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR,
>> CAN_SLEEP);
>>   	} else {
>>   		/* Update the netdev mac with the device's mac */
>>   		memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev-
>>> addr_len);
>> @@ -2830,7 +2830,7 @@ static int ionic_station_set(struct ionic_lif
>> *lif)
>>   
>>   	netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
>>   		   netdev->dev_addr);
>> -	ionic_lif_addr(lif, netdev->dev_addr, true, true);
>> +	ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR, CAN_SLEEP);
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> index 0224dfd24b8a..493de679b498 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> @@ -13,6 +13,14 @@
>>   
>>   #define IONIC_MAX_NUM_NAPI_CNTR		(NAPI_POLL_WEIGHT + 1)
>>   #define IONIC_MAX_NUM_SG_CNTR		(IONIC_TX_MAX_SG_ELEMS
>> + 1)
>> +
>> +#define ADD_ADDR	true
>> +#define DEL_ADDR	false
>> +#define CAN_SLEEP	true
>> +#define CAN_NOT_SLEEP	false
>> +#define FROM_NDO	true
>> +#define NOT_FROM_NDO	false
>> +
>>   #define IONIC_RX_COPYBREAK_DEFAULT	256
>>   #define IONIC_TX_BUDGET_DEFAULT		256
>>