linux-kernel - Re: [PATCH v2 1/4] ACPI / watchdog: Add support for WDAT hardware watchdog

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <03ecb311-45ed-f8e2-1958-868de0ebb7b2@roeck-us.net>
Date:   Tue, 20 Sep 2016 07:48:53 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Wim Van Sebroeck <wim@...ana.be>,
        Len Brown <lenb@...nel.org>, Jean Delvare <jdelvare@...e.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Peter Tyser <ptyser@...-inc.com>,
        Lee Jones <lee.jones@...aro.org>,
        Zha Qipeng <qipeng.zha@...el.com>,
        Darren Hart <dvhart@...radead.org>, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] ACPI / watchdog: Add support for WDAT hardware
 watchdog

On 09/20/2016 06:50 AM, Mika Westerberg wrote:
> On Tue, Sep 20, 2016 at 06:37:23AM -0700, Guenter Roeck wrote:
>>> +	for (i = 0; i < tbl->entries; i++) {
>>> +		const struct acpi_generic_address *gas;
>>> +		struct wdat_instruction *instr;
>>> +		struct list_head *instructions;
>>> +		unsigned int action;
>>> +		struct resource r;
>>> +		int j;
>>> +
>>> +		action = entries[i].action;
>>> +		if (action >= MAX_WDAT_ACTIONS) {
>>> +			dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
>>> +				action);
>>> +			continue;
>>> +		}
>>> +
>>> +		instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
>>> +		if (!instr)
>>> +			return -ENOMEM;
>>> +
>>> +		INIT_LIST_HEAD(&instr->node);
>>> +		instr->entry = entries[i];
>>> +
>>> +		gas = &entries[i].register_region;
>>> +
>>> +		memset(&r, 0, sizeof(r));
>>> +		r.start = gas->address;
>>> +		r.end = r.start + gas->access_width;
>>> +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>>> +			r.flags = IORESOURCE_MEM;
>>> +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
>>> +			r.flags = IORESOURCE_IO;
>>> +		} else {
>>> +			dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
>>> +				gas->space_id);
>>> +			continue;
>>> +		}
>>> +
>>> +		/* Find the matching resource */
>>> +		for (j = 0; j < pdev->num_resources; j++) {
>>> +			res = &pdev->resource[j];
>>> +			if (resource_contains(res, &r)) {
>>> +				instr->reg = regs[j] + r.start - res->start;
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		if (!instr->reg) {
>>> +			dev_err(&pdev->dev, "I/O resource not found\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>> I must be missing something here. What is happening with instr ?
>
> Instr contains the actual instruction from WDAT table. There can be
> many.
>
>>> +		instructions = wdat->instructions[action];
>
> This will be NULL first time so...
>
>>> +		if (!instructions) {
>>> +			instructions = devm_kzalloc(&pdev->dev,
>>> +					sizeof(*instructions), GFP_KERNEL);
>>> +			if (!instructions)
>>> +				return -ENOMEM;
>>> +
>>> +			INIT_LIST_HEAD(instructions);
>>> +			wdat->instructions[action] = instructions;
>>
>> ... and I don't really see how wdat->instructions[action] contains anything but
>> a zero filled array. Confused :-(.
>
> ...we allocate entry for that here. Next time it will not be NULL for
> that action.
>
>>
>>> +		}
>>> +
>>> +		list_add_tail(&instr->node, instructions);
>
> Then we append each instr to the list for this action.
>
> Hope this clarifies :)
>
Yes, I somehow didn't see that last line. Too early in the morning,
not enough coffee.

Thanks,
Guenter