linux-kernel - Re: [PATCH v4] platform/x86: portwell-ec: Add GPIO and WDT driver for Portwell EC

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: <a77a4e1f-7eb6-4194-923d-f8b19000bcf6@portwell.com.tw>
Date: Thu, 17 Apr 2025 18:18:26 +0800
From: jesse huang <jesse.huang@...twell.com.tw>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>, linus.walleij@...aro.org,
 brgl@...ev.pl, wim@...ux-watchdog.org, linux@...ck-us.net,
 LKML <linux-kernel@...r.kernel.org>, platform-driver-x86@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-watchdog@...r.kernel.org,
 jay.chen@...onical.com
Subject: Re: [PATCH v4] platform/x86: portwell-ec: Add GPIO and WDT driver for
 Portwell EC

Hi Ilpo, Guenter,

Thank you for the reviews and the patient corrections.

On 16/04/2025 12:30 pm, Guenter Roeck wrote:
> On 4/15/25 21:10, Yen-Chi Huang wrote:

>> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +    u8 direction = pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset);
>> +
>> +    if (direction)
>> +        return GPIO_LINE_DIRECTION_IN;
>> +    else
>> +        return GPIO_LINE_DIRECTION_OUT;
> 
> Just a side note, you'll get a static analyzer warning telling you
> that else after return is not necessary.
> 
>> +}

Will remove the else in v5.

>> +static int pwec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
>> +{
>> +    if (timeout == 0 || timeout > PORTWELL_WDT_EC_MAX_COUNT_SECOND)
>> +        return -EINVAL;
>> +
> 
> Below:
> 
>> +    .min_timeout = 1,
>> +    .max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,
> 
> This means the watchdog core validates the range. Why check it again here ?
> 
>> +    wdd->timeout = timeout;
>> +    pwec_wdt_write_timeout(wdd->timeout);
>> +
>> +    return 0;
>> +}

Will remove the dummy check in v5. Sorry, I didn't study the core handling closely enough.

>> +    if (!devm_request_region(&pdev->dev, PORTWELL_EC_IOSPACE,
>> +                PORTWELL_EC_IOSPACE_LEN, dev_name(&pdev->dev))) {
>> +        dev_err(&pdev->dev, "I/O region 0xE300-0xE3FF busy\n");
> 
> ... or failed to allocate memory.
> 

Will simplify the message to "failed to get I/O region\n" to make it more general.

---

On 16/04/2025 3:45 pm, Ilpo Jarvinen wrote:
> On Wed, 16 Apr 2025, Yen-Chi Huang wrote:
> 

>> +/* Ensure consistent min/sec read in case of second rollover. */
>> +static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +	u8 min, sec1, sec2;
>> +
>> +	sec1 = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> 
> This is not a safe way to do rollover correction. Lets say rollover 
> happens at this point.
> 
>> +	min = pwec_read(PORTWELL_WDT_EC_COUNT_MIN_ADDR);
>> +	sec2 = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
>> +
>> +	if (sec2 > sec1) {
>> +		min--;
> 
> ...This is subtracting from the post-rollover value.
> 
> This is roughly how drivers/clocksource/ drivers handle rollovers:
> 
> 	min = readmin();
> 	do {
> 		old_min = min;
> 		sec = readsec();
> 		min = readmin();
> 	} while (min != old_min);
> 
>> +		sec1 = sec2;
>> +	}
>> +
>> +	return min * 60 + sec1;
>> +}
>> +

Will rework in v5.

Appreciate the reference and the guidance, and sorry
for not getting it right the first time.

>> +	return (!strcmp(PORTWELL_EC_FW_VENDOR_NAME, buf)) ? 0 : -ENODEV;
> 
> Those parenthesis are unnecessary.

Will remove the parenthesis in v5.

>> +static struct platform_driver pwec_driver = {
>> +	.driver = {
>> +		.name = "portwell-ec",
>> +	},
>> +	.probe = pwec_probe
> 
> Please include the comma to any entry that is not a terminator entry to 
> make diffs cleaner if another member is added here later.

Will add the comma in v5. Apologies for missing it in the previous version.

>> +static void __exit pwec_exit(void)
>> +{
>> +	if (pwec_dev)
> 
> How can pmec_dev be NULL if init always sets it or returns error?

Will remove it in v5.

Best regards,  
Yen-Chi Huang