linux-kernel - Re: [PATCH 2/2] scsi: libsas: Use bool for queue

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: <93ca25b8-db43-3ce8-0923-3acad41d0856@huawei.com>
Date:   Thu, 24 Feb 2022 13:07:44 +0000
From:   John Garry <john.garry@...wei.com>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        <jejb@...ux.ibm.com>, <martin.petersen@...cle.com>
CC:     <chenxiang66@...ilicon.com>, <linux-scsi@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>
Subject: Re: [PATCH 2/2] scsi: libsas: Use bool for queue_work() return code

>> -int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>> +bool sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>>   {
>>   	/* it's added to the defer_q when draining so return succeed */
>> -	int rc = 1;
>> +	bool rc = true;
>>   
>>   	if (!test_bit(SAS_HA_REGISTERED, &ha->state))
>> -		return 0;
>> +		return false;
>>   
>>   	if (test_bit(SAS_HA_DRAINING, &ha->state)) {
>>   		/* add it to the defer list, if not already pending */
>> @@ -28,11 +28,11 @@ int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>>   	return rc;
> 
> While at it, I would cleanup this function like this:

ok, fine

> 
> diff --git a/drivers/scsi/libsas/sas_event.c
> b/drivers/scsi/libsas/sas_event.c
> index 3613b9b315bc..38e6e91aaf36 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -12,20 +12,17 @@
> 
>   int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>   {
> -       /* it's added to the defer_q when draining so return succeed */
> -       int rc = 1;
> -
>          if (!test_bit(SAS_HA_REGISTERED, &ha->state))
> -               return 0;
> +               return false;
> 
>          if (test_bit(SAS_HA_DRAINING, &ha->state)) {
>                  /* add it to the defer list, if not already pending */
>                  if (list_empty(&sw->drain_node))
>                          list_add_tail(&sw->drain_node, &ha->defer_q);
> -       } else
> -               rc = queue_work(ha->event_q, &sw->work);
> +               return true;
> +       }
> 
> -       return rc;
> +       return queue_work(ha->event_q, &sw->work);
>   }
> 
> No local variable :)
> 
>>   }
>>   
>> -static int sas_queue_event(int event, struct sas_work *work,
>> +static bool sas_queue_event(int event, struct sas_work *work,
>>   			    struct sas_ha_struct *ha)
>>   {
>>   	unsigned long flags;
>> -	int rc;
>> +	bool rc;
>>   
>>   	spin_lock_irqsave(&ha->lock, flags);
>>   	rc = sas_queue_work(ha, work);
>> @@ -44,13 +44,12 @@ static int sas_queue_event(int event, struct sas_work *work,
>>   void sas_queue_deferred_work(struct sas_ha_struct *ha)
>>   {
>>   	struct sas_work *sw, *_sw;
>> -	int ret;
>>   
>>   	spin_lock_irq(&ha->lock);
>>   	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>>   		list_del_init(&sw->drain_node);
>> -		ret = sas_queue_work(ha, sw);
>> -		if (ret != 1) {
>> +
>> +		if (sas_queue_work(ha, sw) == false) {
> 
> if (!sas_queue_work(ha, sw)) ?

ok, yeah, that's the pattern I see elsehwhere in the kernel

> 
>>   			pm_runtime_put(ha->dev);
>>   			sas_free_event(to_asd_sas_event(&sw->work));
>>   		}
>> @@ -170,7 +169,6 @@ void sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
>>   {
>>   	struct sas_ha_struct *ha = phy->ha;
>>   	struct asd_sas_event *ev;
>> -	int ret;
>>   
>>   	BUG_ON(event >= PORT_NUM_EVENTS);
>>   
>> @@ -186,8 +184,7 @@ void sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
>>   	if (sas_defer_event(phy, ev))
>>   		return;
>>   
>> -	ret = sas_queue_event(event, &ev->work, ha);
>> -	if (ret != 1) {
>> +	if (sas_queue_event(event, &ev->work, ha) == false) {
> 
> Same.
> 
>>   		pm_runtime_put(ha->dev);
>>   		sas_free_event(ev);
>>   	}
>> @@ -199,7 +196,6 @@ void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
>>   {
>>   	struct sas_ha_struct *ha = phy->ha;
>>   	struct asd_sas_event *ev;
>> -	int ret;
>>   
>>   	BUG_ON(event >= PHY_NUM_EVENTS);
>>   
>> @@ -215,8 +211,7 @@ void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
>>   	if (sas_defer_event(phy, ev))
>>   		return;
>>   
>> -	ret = sas_queue_event(event, &ev->work, ha);
>> -	if (ret != 1) {
>> +	if (sas_queue_event(event, &ev->work, ha) == false) {
> 
> And again.
> 

ok, thanks