Received-SPF: pass (google.com: domain of linux-wireless+bounces-6674-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <f30ad02c-0c52-4a28-b63f-22ae84f23b77@quicinc.com>
Date: Mon, 22 Apr 2024 09:09:46 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>
CC: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>,
        <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240409151415.1226109-1-quic_periyasa@quicinc.com>
 <20240409151415.1226109-5-quic_periyasa@quicinc.com>
 <877ch4z0z1.fsf@kernel.org>
 <6ac588c2-6206-c12e-e741-cbc507365bc3@quicinc.com>
 <823fe8a6-1e6c-49e9-a225-2012e77093ac@quicinc.com>
 <87y195vc0w.fsf@kernel.org>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <87y195vc0w.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/22/2024 5:04 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> 
>>>>> +	default:
>>>>> +		ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	/* Write to PPT in CMEM */
>>>>> +	for (i = start; i < end; i++)
>>>>> +		ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i),
>>>>> + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET);
>>>>> +}
>>>>
>>>> Here's a good example why I don't like functions returning void. How do
>>>> we handle the errors in this case?
>>>>
>>>
>>> sure, will handle the error case in the caller.
>>>
>>
>> this is a static function with one caller. the only error is the default case
>> which will never be hit. adding logic to return an error and then check it in
>> the caller seems like overkill. why not just WARN() in the default case since
>> this would be a logic error with newly added code?
> 
> I think the software will be more robust then all errors are properly
> handled in a uniform way. For example, will everyone notice the warning
> message? What if the function is extended later and then the person
> doesn't add any error handling "because it didn't have that even
> earlier"? It's also a lot easier to review if error handling follows the
> same style throughout the driver.

A large number of coding errors occur in exception paths. So minimizing the
number of exception paths decreases the opportunity for introducing these kind
of errors. So the real trick is making sure "all errors are properly handled
in a uniform way"

/jeff