From: ilkka@os.amperecomputing.com To: "Russell King (Oracle)" <linux@armlinux.org.uk> Cc: Ilkka Koskinen <ilkka@os.amperecomputing.com>, lorenzo.pieralisi@arm.com, guohanjun@huawei.com, sudeep.holla@arm.com, rafael@kernel.org, lenb@kernel.org, robert.moore@intel.com, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devel@acpica.org, patches@amperecomputing.com, scott@os.amperecomputing.com, darren@os.amperecomputing.com Subject: Re: [PATCH 2/2] ACPI: AGDI: Add driver for Arm Generic Diagnostic Dump and Reset device Date: Fri, 10 Dec 2021 00:32:09 -0800 (PST) [thread overview] Message-ID: <alpine.DEB.2.22.394.2112092359001.2491@ubuntu200401> (raw) In-Reply-To: <YbIwfzND2R4WyQO7@shell.armlinux.org.uk> Hi, Thanks for reviewing the patch On Thu, 9 Dec 2021, Russell King (Oracle) wrote: > Hi, > > On Thu, Dec 02, 2021 at 06:43:11PM -0800, Ilkka Koskinen wrote: >> +static int __init agdi_init(void) >> +{ >> + int ret; >> + acpi_status status; >> + struct acpi_table_agdi *agdi_table; >> + struct agdi_data *pdata; >> + struct platform_device *pdev; >> + >> + if (acpi_disabled) >> + return 0; >> + >> + status = acpi_get_table(ACPI_SIG_AGDI, 0, >> + (struct acpi_table_header **) &agdi_table); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + pdata = kzalloc(sizeof(*pdata), GFP_ATOMIC); > > Why does this need to be GFP_ATOMIC? Also, struct agdi_data is a single > int in size, why do you need to kzalloc() it? There's no reason for that. It should obviously be GFP_KERNEL > >> + if (!pdata) { >> + ret = -ENOMEM; >> + goto err_put_table; >> + } >> + >> + if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) { >> + pr_warn("Interrupt signaling is not supported"); >> + ret = -ENODEV; >> + goto err_free_pdata; >> + } >> + >> + pdata->sdei_event = agdi_table->sdei_event; >> + >> + pdev = platform_device_register_data(NULL, "agdi", 0, pdata, sizeof(*pdata)); > > platform_device_register_data() uses kmemdup() internally with the > platform data, meaning it takes a copy of the platform data. There is > no need for the pdata allocation to persist past this point. Hence, > given that it is a single int in size, you may as well put > "struct agdi_data" on the stack. I somehow missed kmemdup() part. I remove the allocation and move pdata to the stack instead. Thanks, Ilkka > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
WARNING: multiple messages have this Message-ID (diff)
From: ilkka@os.amperecomputing.com To: "Russell King (Oracle)" <linux@armlinux.org.uk> Cc: Ilkka Koskinen <ilkka@os.amperecomputing.com>, lorenzo.pieralisi@arm.com, guohanjun@huawei.com, sudeep.holla@arm.com, rafael@kernel.org, lenb@kernel.org, robert.moore@intel.com, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devel@acpica.org, patches@amperecomputing.com, scott@os.amperecomputing.com, darren@os.amperecomputing.com Subject: Re: [PATCH 2/2] ACPI: AGDI: Add driver for Arm Generic Diagnostic Dump and Reset device Date: Fri, 10 Dec 2021 00:32:09 -0800 (PST) [thread overview] Message-ID: <alpine.DEB.2.22.394.2112092359001.2491@ubuntu200401> (raw) In-Reply-To: <YbIwfzND2R4WyQO7@shell.armlinux.org.uk> Hi, Thanks for reviewing the patch On Thu, 9 Dec 2021, Russell King (Oracle) wrote: > Hi, > > On Thu, Dec 02, 2021 at 06:43:11PM -0800, Ilkka Koskinen wrote: >> +static int __init agdi_init(void) >> +{ >> + int ret; >> + acpi_status status; >> + struct acpi_table_agdi *agdi_table; >> + struct agdi_data *pdata; >> + struct platform_device *pdev; >> + >> + if (acpi_disabled) >> + return 0; >> + >> + status = acpi_get_table(ACPI_SIG_AGDI, 0, >> + (struct acpi_table_header **) &agdi_table); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + pdata = kzalloc(sizeof(*pdata), GFP_ATOMIC); > > Why does this need to be GFP_ATOMIC? Also, struct agdi_data is a single > int in size, why do you need to kzalloc() it? There's no reason for that. It should obviously be GFP_KERNEL > >> + if (!pdata) { >> + ret = -ENOMEM; >> + goto err_put_table; >> + } >> + >> + if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) { >> + pr_warn("Interrupt signaling is not supported"); >> + ret = -ENODEV; >> + goto err_free_pdata; >> + } >> + >> + pdata->sdei_event = agdi_table->sdei_event; >> + >> + pdev = platform_device_register_data(NULL, "agdi", 0, pdata, sizeof(*pdata)); > > platform_device_register_data() uses kmemdup() internally with the > platform data, meaning it takes a copy of the platform data. There is > no need for the pdata allocation to persist past this point. Hence, > given that it is a single int in size, you may as well put > "struct agdi_data" on the stack. I somehow missed kmemdup() part. I remove the allocation and move pdata to the stack instead. Thanks, Ilkka > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-12-10 8:32 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-03 2:43 [PATCH 0/2] ACPI: Arm Generic Diagnostic Dump and Reset device Ilkka Koskinen 2021-12-03 2:43 ` Ilkka Koskinen 2021-12-03 2:43 ` [PATCH 1/2] ACPI: AGDI: Add AGDI tables to drivers/acpi Ilkka Koskinen 2021-12-03 2:43 ` Ilkka Koskinen 2021-12-03 2:43 ` [PATCH 2/2] ACPI: AGDI: Add driver for Arm Generic Diagnostic Dump and Reset device Ilkka Koskinen 2021-12-03 2:43 ` Ilkka Koskinen 2021-12-09 16:36 ` Russell King (Oracle) 2021-12-09 16:36 ` Russell King (Oracle) 2021-12-10 8:32 ` ilkka [this message] 2021-12-10 8:32 ` ilkka
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.DEB.2.22.394.2112092359001.2491@ubuntu200401 \ --to=ilkka@os.amperecomputing.com \ --cc=darren@os.amperecomputing.com \ --cc=devel@acpica.org \ --cc=guohanjun@huawei.com \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=lorenzo.pieralisi@arm.com \ --cc=patches@amperecomputing.com \ --cc=rafael@kernel.org \ --cc=robert.moore@intel.com \ --cc=scott@os.amperecomputing.com \ --cc=sudeep.holla@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.