On 04/30/2015 10:30 PM, Guenter Roeck wrote:
+/* Watchdog Refresh Frame */ +struct arm_sbsa_watchdog_refresh {
- uint32_t wrr; /* Watchdog Refresh Register */
- uint8_t res1[0xFCC - 0x004];
- struct arm_sbsa_watchdog_ident ident;
+};
+/* Watchdog Control Frame */ +struct arm_sbsa_watchdog_control {
- uint32_t wcs;
- uint32_t res1;
- uint32_t wor;
- uint32_t res2;
- uint64_t wcv;
- uint8_t res3[0xFCC - 0x018];
- struct arm_sbsa_watchdog_ident ident;
+};
Why not just use defines instead of all those structures ?
I like structures. I think hardware register blocks should be defined with structures that provide type checking.
+static int arm_sbsa_wdt_set_timeout(struct watchdog_device *wdev,
- unsigned int timeout)
+{
- struct arm_sbsa_watchdog_data *data =
container_of(wdev, struct arm_sbsa_watchdog_data, wdev);- wdev->timeout = timeout;
- writel(arch_timer_get_rate() * wdev->timeout, &data->control->wor);
Do you also have to reset the counter ?
No. Programming a new timeout automatically resets the timer.
+static int __init arm_sbsa_wdt_probe(struct platform_device *pdev) +{
- struct arm_sbsa_watchdog_data *data;
- struct resource *res;
- uint32_t iidr;
- int ret;
- data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
- if (!data)
return -ENOMEM;- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res || !res->start) {
dev_err(&pdev->dev, "could not get control address\n");return -ENOMEM;- }
devm_ioremap_resource already prints an error message if res is NULL. And res->start can not be 0 unless there is a severe infrastructure problem.
Will fix.
- data->control = devm_ioremap_resource(&pdev->dev, res);
- if (!data->control)
return -ENOMEM;- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- if (!res || !res->start) {
dev_err(&pdev->dev, "could not get refresh address\n");return -ENOMEM;- }
Same here.
- data->refresh = devm_ioremap_resource(&pdev->dev, res);
- if (!data->refresh)
return -ENOMEM;- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res || !res->start) {
res->start checking is unnecessary. On a side note, irq == 0 might be a valid interrupt number.
Will fix.
dev_err(&pdev->dev, "could not get interrupt\n");return -ENOMEM;- }
- iidr = readl(&data->control->ident.w_iidr);
- /* We only support architecture version 0 */
- if (((iidr >> 16) & 0xf) != 0) {
dev_info(&pdev->dev, "only architecture version 0 issupported\n");
return -ENODEV;- }
- ret = devm_request_irq(&pdev->dev, res->start,
arm_sbsa_wdt_interrupt,
0, DRV_NAME, NULL);Please align continuation lines with '('.
Will fix.
- if (ret < 0) {
dev_err(&pdev->dev, "could not request irq %u (ret=%i)\n",(unsigned int)res->start, ret);return ret;- }
- data->wdev.info = &arm_sbsa_wdt_info;
- data->wdev.ops = &arm_sbsa_wdt_ops;
- data->wdev.min_timeout = 1;
- data->wdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
- /* Calculate the maximum timeout in seconds that we can support */
- data->wdev.max_timeout = U32_MAX / arch_timer_get_rate();
- ret = watchdog_register_device(&data->wdev);
- if (ret < 0) {
dev_err(&pdev->dev,"could not register watchdog device (ret=%i)\n", ret);return ret;- }
- dev_info(&pdev->dev, "implementer code is %03x, max timeout is %u
seconds\n",
(iidr & 0xf00) >> 1 | (iidr & 0x7f), data->wdev.max_timeout);"Implementer code" sounds very much like a debug message to me. It means nothing to me, and it won't mean anything to a user.
Fair enough. I thought it would be a handy way to know that the driver found the hardware, but I'll move it to a pr_debug().
- /*
* Bits [15:12] are an implementation-defined revision number* for the component.*/- arm_sbsa_wdt_info.firmware_version = (iidr >> 12) & 0xf;
It might make sense to set that prior to registering the driver.
Ok.
+static struct platform_device *arm_sbsa_wdt_pdev;
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header *header,
- const unsigned long end)
+{
- struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog
*)header;
- struct platform_device *arm_sbsa_wdt_pdev;
That is an interesting one. Makes me wonder if you ever tried to unload this driver. Did you ?
The driver can only be compiled in-kernel.
- struct resource res[3] = {};
- int trigger, polarity;
- int ret;
- if (!header ||
That error check is kind of weird. Sure, 0 is an invalid address, but so are many other addresses. Is there any reason to believe that acpi_get_table_with_size would return a NULL pointer (but not some other invalid address) ?
If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.
(unsigned long)header + sizeof(*wdg) > end ||header->length < sizeof(*wdg)) {So I really wonder how this is supposed to work.
struct acpi_subtable_header { u8 type; u8 length; };
struct acpi_gtdt_watchdog { struct acpi_gtdt_header header; ...
struct acpi_gtdt_header { u8 type; u16 length; };
Either the length is 16 bit or 8 bit. But both ??? Or is it just luck and the value happens to be stored as little endian value and the length is less than 256 bytes ?
I understand this seems to be copied from BAD_MADT_ENTRY or similar code, but it is still odd.
It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.
pr_err("malformed subtable entry\n");return -EINVAL;- }
- if (!wdg->control_frame_address || !wdg->refresh_frame_address) {
pr_err("invalid control or refresh address\n");return -ENXIO;- }
- arm_sbsa_wdt_pdev = platform_device_alloc(DRV_NAME, 0);
- if (!arm_sbsa_wdt_pdev)
return -ENOMEM;- res[0].name = "control";
- res[0].flags = IORESOURCE_MEM;
- res[0].start = wdg->control_frame_address;
- res[0].end = res[0].start + sizeof(struct
arm_sbsa_watchdog_control);
Really ? Or should there be a -1 somewhere ?
You're right.
- res[1].name = "refresh";
- res[1].flags = IORESOURCE_MEM;
- res[1].start = wdg->refresh_frame_address;
- res[1].end = res[1].start + sizeof(struct
arm_sbsa_watchdog_refresh);
Same here.
Ok.
- trigger = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_MODE) ?
ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;- polarity = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_POLARITY) ?
ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;- /* This should be the WS0 interrupt */
- ret = acpi_register_gsi(&arm_sbsa_wdt_pdev->dev,
wdg->timer_interrupt,
trigger, polarity);- if (ret <= 0) {
0 is not an error (the interrupt number could be 0).
Ok.
+static int __init arm_sbsa_wdt_init(void) +{
- struct acpi_table_header *table;
- acpi_size tbl_size;
- acpi_status status;
- int count;
- pr_info("ARM Server Base System Architecture watchdog driver\n");
This seems to assume that the watchdog is always supported, which is quite unlikely.
I'll make it a pr_debug.
- status = acpi_get_table_with_size(ACPI_SIG_GTDT, 0, &table,
&tbl_size);
- if (ACPI_FAILURE(status)) {
pr_err("cannot locate GTDT table\n");return -EINVAL;- }
I am kind of completely unhappy with the noisiness here and below. Is this how acpi detection is supposed to happen ? And is it really an _error_ if the device isn't there, or does it just mean that the device isn't there ?
I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.
However, I'm not an ACPI expert by any means. I'm hoping that someone who knows a lot more than I do will review it. This driver was reviewed and approved by some of our internal ACPI experts, so I presume that it is correct.
- count = acpi_parse_entries(ACPI_SIG_GTDT, sizeof(struct
acpi_table_gtdt),
arm_sbsa_wdt_parse_gtdt, table, ACPI_GTDT_TYPE_WATCHDOG, 0);- if (count <= 0) {
pr_err("no watchdog subtables found\n");return -EINVAL;- }
- return platform_driver_probe(&arm_sbsa_wdt_driver,
arm_sbsa_wdt_probe);
Another oddity. Is this how acpi device instantiation is supposed to happen ?
I thought there are functions to register acpi drivers, such as acpi_bus_register_driver(). Why doesn't this work here ?
I can't use acpi_bus_register_driver() because there are no ACPI IDs to probe on. Watchdog information is stored as a subtable of the GTDT table, and there's nothing in the ACPI layer that automatically creates platforms from subtables. I have to create the platform from scratch, which is why the code looks so messy.
On 05/01/2015 11:16 AM, Timur Tabi wrote:
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res || !res->start) {
dev_err(&pdev->dev, "could not get control address\n");return -ENOMEM;- }
devm_ioremap_resource already prints an error message if res is NULL. And res->start can not be 0 unless there is a severe infrastructure problem.
Will fix.
- data->control = devm_ioremap_resource(&pdev->dev, res);
- if (!data->control)
return -ENOMEM;- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- if (!res || !res->start) {
dev_err(&pdev->dev, "could not get refresh address\n");return -ENOMEM;- }
Same here.
So I must be missing something here. I'm only printing an error message if platform_get_resource() fails. I'm not printing a message if devm_ioremap_resource() fails. Are you saying that I should not print an error message if platform_get_resource() fails? What's wrong with that?
On Fri, May 01, 2015 at 12:28:48PM -0500, Timur Tabi wrote:
On 05/01/2015 11:16 AM, Timur Tabi wrote:
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res || !res->start) {
dev_err(&pdev->dev, "could not get control address\n");return -ENOMEM;- }
devm_ioremap_resource already prints an error message if res is NULL. And res->start can not be 0 unless there is a severe infrastructure problem.
Will fix.
- data->control = devm_ioremap_resource(&pdev->dev, res);
- if (!data->control)
return -ENOMEM;- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- if (!res || !res->start) {
dev_err(&pdev->dev, "could not get refresh address\n");return -ENOMEM;- }
Same here.
So I must be missing something here. I'm only printing an error message if platform_get_resource() fails. I'm not printing a message if devm_ioremap_resource() fails. Are you saying that I should not print an error message if platform_get_resource() fails? What's wrong with that?
devm_ioremap_resource already prints a message. For this reason, elsewhere in the kernel the check for !res before calling devm_ioremap_resource is being removed, leaving the error handling to devm_ioremap_resource. I would suggest to do the same here.
Guenter
On 05/01/2015 12:32 PM, Guenter Roeck wrote:
devm_ioremap_resource already prints a message. For this reason, elsewhere in the kernel the check for !res before calling devm_ioremap_resource is being removed, leaving the error handling to devm_ioremap_resource. I would suggest to do the same here.
I see what you're saying, but I leave the error handling to devm_ioremap_resource(), then no one will know whether it's because the control address or the refresh address is missing. The user will just see "invalid resource" and won't what resource is actually invalid.
On Fri, May 01, 2015 at 11:16:41AM -0500, Timur Tabi wrote:
On 04/30/2015 10:30 PM, Guenter Roeck wrote:
+/* Watchdog Refresh Frame */ +struct arm_sbsa_watchdog_refresh {
- uint32_t wrr; /* Watchdog Refresh Register */
- uint8_t res1[0xFCC - 0x004];
- struct arm_sbsa_watchdog_ident ident;
+};
+/* Watchdog Control Frame */ +struct arm_sbsa_watchdog_control {
- uint32_t wcs;
- uint32_t res1;
- uint32_t wor;
- uint32_t res2;
- uint64_t wcv;
- uint8_t res3[0xFCC - 0x018];
- struct arm_sbsa_watchdog_ident ident;
+};
Why not just use defines instead of all those structures ?
I like structures. I think hardware register blocks should be defined with structures that provide type checking.
Matter of personal opinion, I guess. Also prone to introducing errors, and potentially resulting in more complex code. Just my personal opinion, though, so I'll let that pass.
[ ... ]
+static struct platform_device *arm_sbsa_wdt_pdev;
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header *header,
- const unsigned long end)
+{
- struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog
*)header;
- struct platform_device *arm_sbsa_wdt_pdev;
That is an interesting one. Makes me wonder if you ever tried to unload this driver. Did you ?
The driver can only be compiled in-kernel.
And that makes it valid to have both a global and a local variable named arm_sbsa_wdt_pdev ? Please explain the rationale.
- struct resource res[3] = {};
- int trigger, polarity;
- int ret;
- if (!header ||
That error check is kind of weird. Sure, 0 is an invalid address, but so are many other addresses. Is there any reason to believe that acpi_get_table_with_size would return a NULL pointer (but not some other invalid address) ?
If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.
How would it be uninitialized ? A quick glance to other code seems to suggest that this isn't needed elsewhere. Why is it needed here ?
(unsigned long)header + sizeof(*wdg) > end ||header->length < sizeof(*wdg)) {So I really wonder how this is supposed to work.
struct acpi_subtable_header { u8 type; u8 length; };
struct acpi_gtdt_watchdog { struct acpi_gtdt_header header; ...
struct acpi_gtdt_header { u8 type; u16 length; };
Either the length is 16 bit or 8 bit. But both ??? Or is it just luck and the value happens to be stored as little endian value and the length is less than 256 bytes ?
I understand this seems to be copied from BAD_MADT_ENTRY or similar code, but it is still odd.
It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.
That doesn't explain if length is supposed to be 16 or 8 bit, if the length is supposed to be stored as big or little endian value, and what would happen if it was stored as big endian value on some system and is 16 bit wide.
If the length is in fact 16 bit, you could just check its value directly, instead of doing all the typecasting. And if it is 16 bit and has a fixed endianness (not host byte order), you should convert it to host byte order before validating it. Alternatively, something appears to be wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog.
pr_err("malformed subtable entry\n");return -EINVAL;- }
- if (!wdg->control_frame_address || !wdg->refresh_frame_address) {
pr_err("invalid control or refresh address\n");return -ENXIO;- }
- arm_sbsa_wdt_pdev = platform_device_alloc(DRV_NAME, 0);
- if (!arm_sbsa_wdt_pdev)
return -ENOMEM;- res[0].name = "control";
- res[0].flags = IORESOURCE_MEM;
- res[0].start = wdg->control_frame_address;
- res[0].end = res[0].start + sizeof(struct
arm_sbsa_watchdog_control);
Really ? Or should there be a -1 somewhere ?
You're right.
- res[1].name = "refresh";
- res[1].flags = IORESOURCE_MEM;
- res[1].start = wdg->refresh_frame_address;
- res[1].end = res[1].start + sizeof(struct
arm_sbsa_watchdog_refresh);
Same here.
Ok.
- trigger = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_MODE) ?
ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;- polarity = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_POLARITY) ?
ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;- /* This should be the WS0 interrupt */
- ret = acpi_register_gsi(&arm_sbsa_wdt_pdev->dev,
wdg->timer_interrupt,
trigger, polarity);- if (ret <= 0) {
0 is not an error (the interrupt number could be 0).
Ok.
+static int __init arm_sbsa_wdt_init(void) +{
- struct acpi_table_header *table;
- acpi_size tbl_size;
- acpi_status status;
- int count;
- pr_info("ARM Server Base System Architecture watchdog driver\n");
This seems to assume that the watchdog is always supported, which is quite unlikely.
I'll make it a pr_debug.
- status = acpi_get_table_with_size(ACPI_SIG_GTDT, 0, &table,
&tbl_size);
- if (ACPI_FAILURE(status)) {
pr_err("cannot locate GTDT table\n");return -EINVAL;- }
I am kind of completely unhappy with the noisiness here and below. Is this how acpi detection is supposed to happen ? And is it really an _error_ if the device isn't there, or does it just mean that the device isn't there ?
I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.
ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?
Assuming that there can be an image which boots both with ACPI and devicetree based configurations, I can understand that this driver would only load on ACPI based arm64 systems, but that doesn't mean it should dump an error message if the system does not use ACPI (or if its ACPI tables do not include an entry for the watchdog, for that matter).
However, I'm not an ACPI expert by any means. I'm hoping that someone who knows a lot more than I do will review it. This driver was reviewed and approved by some of our internal ACPI experts, so I presume that it is correct.
- count = acpi_parse_entries(ACPI_SIG_GTDT, sizeof(struct
acpi_table_gtdt),
arm_sbsa_wdt_parse_gtdt, table, ACPI_GTDT_TYPE_WATCHDOG, 0);- if (count <= 0) {
pr_err("no watchdog subtables found\n");return -EINVAL;- }
- return platform_driver_probe(&arm_sbsa_wdt_driver,
arm_sbsa_wdt_probe);
Another oddity. Is this how acpi device instantiation is supposed to happen ?
I thought there are functions to register acpi drivers, such as acpi_bus_register_driver(). Why doesn't this work here ?
I can't use acpi_bus_register_driver() because there are no ACPI IDs to probe on. Watchdog information is stored as a subtable of the GTDT table, and there's nothing in the ACPI layer that automatically creates platforms from subtables. I have to create the platform from scratch, which is why the code looks so messy.
Weird. I'll really want to know from some ACPI experts if this is really how ACPI drivers are supposed to be instantiated. Can you give me some other examples where this is done ?
Thanks, Guenter
On Fri, May 01, 2015 at 12:41:59PM -0500, Timur Tabi wrote:
On 05/01/2015 12:32 PM, Guenter Roeck wrote:
devm_ioremap_resource already prints a message. For this reason, elsewhere in the kernel the check for !res before calling devm_ioremap_resource is being removed, leaving the error handling to devm_ioremap_resource. I would suggest to do the same here.
I see what you're saying, but I leave the error handling to devm_ioremap_resource(), then no one will know whether it's because the control address or the refresh address is missing. The user will just see "invalid resource" and won't what resource is actually invalid.
You are saying that pretty much everyone in the kernel is, in your opinion, doing the Wrong Thing (tm) and you insist in doing it differently.
As maintainer, I have seen lots of patches which remove this very same error checking as unnecessary. If we accept your code, we can be all but sure to see such a patch at some point, probably right after your patch was accepted and shows up in linux-next. So besides arguing about something we should not have to argue about in the first place, you are trying to create even more maintainer work going forward.
Is that really neceessary ?
And then people wonder why maintainers sometimes get grumpy :-(.
Guenter
On 05/01/2015 12:49 PM, Guenter Roeck wrote:
That is an interesting one. Makes me wonder if you ever tried to unload this driver. Did you ?
The driver can only be compiled in-kernel.
And that makes it valid to have both a global and a local variable named arm_sbsa_wdt_pdev ? Please explain the rationale.
Because the driver has to create the ACPI platform device, it needs to call functions that are not exported:
ERROR: "acpi_parse_entries" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_get_rate" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_read_counter" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.
How would it be uninitialized ? A quick glance to other code seems to suggest that this isn't needed elsewhere. Why is it needed here ?
I don't have a good answer to that, except that when I wrote this code, I wanted to add some error checking for the ACPI tables, and this is what I came up with.
If you're telling me that the code is wrong and it will generate false positives, then I can fix it. But if you're telling me that you don't understand why I'm doing some error checking on the ACPI tables that I'm parsing, well, I don't understand what could be wrong with that.
It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.
That doesn't explain if length is supposed to be 16 or 8 bit, if the length is supposed to be stored as big or little endian value, and what would happen if it was stored as big endian value on some system and is 16 bit wide.
All lengths and endianness of the fields in the ACPI structures are already defined in the ACPI spec, so that stuff is fixed and already known. I'm not sure what you're getting at. I'm just doing some basic error checking, no different than any of the code that calls BAD_MADT_ENTRY does.
In fact, I suspect that the only reason that BAD_GTDT_ENTRY does not yet exist is because there isn't much support for ACPI timers in the kernel yet.
If the length is in fact 16 bit, you could just check its value directly, instead of doing all the typecasting. And if it is 16 bit and has a fixed endianness (not host byte order), you should convert it to host byte order before validating it. Alternatively, something appears to be wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog.
I don't understand what you mean when you say something is wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog. These structures look perfectly fine to me, and they work. My driver successfully loads and parses the ACPI tables on a real ARM64 server system.
I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.
ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?
Yes, ACPI for ARM64 *servers* is mandatory. ARM64 servers are not supposed to use device tree.
And since this is a driver for SBSA systems (SBSA = Server Base System Architecture), this driver will only ever be used on an ARM64 server system with ACPI and no device tree at all.
Assuming that there can be an image which boots both with ACPI and devicetree based configurations, I can understand that this driver would only load on ACPI based arm64 systems, but that doesn't mean it should dump an error message if the system does not use ACPI (or if its ACPI tables do not include an entry for the watchdog, for that matter).
I can have the driver silently exit if the GTDT table is missing. However, if it does exist, then all the other error messages are valid.
I can't use acpi_bus_register_driver() because there are no ACPI IDs to probe on. Watchdog information is stored as a subtable of the GTDT table, and there's nothing in the ACPI layer that automatically creates platforms from subtables. I have to create the platform from scratch, which is why the code looks so messy.
Weird. I'll really want to know from some ACPI experts if this is really how ACPI drivers are supposed to be instantiated. Can you give me some other examples where this is done ?
A lot of this code is taken from the GIC driver, which is the closest match. There really isn't much similar, because there's little code that supports the ACPI tables as-is. A lot of the ACPI data is in the form of device nodes, which are device-tree like and are probed very differently.
On 05/01/2015 12:59 PM, Guenter Roeck wrote:
You are saying that pretty much everyone in the kernel is, in your opinion, doing the Wrong Thing (tm) and you insist in doing it differently.
No, of course not, it's just that I've written code like this many times, and no one else ever complained about my error messages before.
As maintainer, I have seen lots of patches which remove this very same error checking as unnecessary. If we accept your code, we can be all but sure to see such a patch at some point, probably right after your patch was accepted and shows up in linux-next. So besides arguing about something we should not have to argue about in the first place, you are trying to create even more maintainer work going forward.
Ok, ok! I'll remove it. I just trying to understand the rationale. I don't agree with it, but I'll make the change you want.
On Friday 01 May 2015 11:16:41 Timur Tabi wrote:
On 04/30/2015 10:30 PM, Guenter Roeck wrote:
+/* Watchdog Refresh Frame */ +struct arm_sbsa_watchdog_refresh {
- uint32_t wrr; /* Watchdog Refresh Register */
- uint8_t res1[0xFCC - 0x004];
- struct arm_sbsa_watchdog_ident ident;
+};
+/* Watchdog Control Frame */ +struct arm_sbsa_watchdog_control {
- uint32_t wcs;
- uint32_t res1;
- uint32_t wor;
- uint32_t res2;
- uint64_t wcv;
- uint8_t res3[0xFCC - 0x018];
- struct arm_sbsa_watchdog_ident ident;
+};
Why not just use defines instead of all those structures ?
I like structures. I think hardware register blocks should be defined with structures that provide type checking.
You should use endian-annotations in the structure if you do this, and mark the members as __le32 or __be32, depending on how the hardware is defined.
+static struct platform_device *arm_sbsa_wdt_pdev;
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header *header,
- const unsigned long end)
+{
- struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog
*)header;
- struct platform_device *arm_sbsa_wdt_pdev;
That is an interesting one. Makes me wonder if you ever tried to unload this driver. Did you ?
The driver can only be compiled in-kernel.
That should be fixed, we want normal drivers to be loadable modules.
Arnd
On Friday 01 May 2015 13:42:41 Timur Tabi wrote:
I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.
ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?
Yes, ACPI for ARM64 *servers* is mandatory. ARM64 servers are not supposed to use device tree.
And since this is a driver for SBSA systems (SBSA = Server Base System Architecture), this driver will only ever be used on an ARM64 server system with ACPI and no device tree at all.
No, that is not a reasonable assumption to make. All the ARM64 server systems we have today are using DT only and we will of course keep supporting systems like that.
Nothing prevents you from using the same register set on a platform that is not SBSA compliant, or using DT on a machine that is SBSA compliant, and we will definitely see that happen for chips that are shared between products, e.g. a SBSA+SCPI system in a server targeted market and the same hardware with devicetree for e.g. networking equipment.
This means we need a DT binding for the driver, and the ACPI portion that creates the platform device should be split out from the main driver.
Arnd
On Fri, May 01, 2015 at 01:42:41PM -0500, Timur Tabi wrote:
On 05/01/2015 12:49 PM, Guenter Roeck wrote:
That is an interesting one. Makes me wonder if you ever tried to unload this driver. Did you ?
The driver can only be compiled in-kernel.
And that makes it valid to have both a global and a local variable named arm_sbsa_wdt_pdev ? Please explain the rationale.
Because the driver has to create the ACPI platform device, it needs to call functions that are not exported:
ERROR: "acpi_parse_entries" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_get_rate" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_read_counter" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
I can understand why that mandates that the driver has to be built into the kernel, at least if you and/or the arm64 maintainers don't want to export those functions. However, I still don't understand why you would need arm_sbsa_wdt_pdev declared twice.
What am I missing here ?
If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.
How would it be uninitialized ? A quick glance to other code seems to suggest that this isn't needed elsewhere. Why is it needed here ?
I don't have a good answer to that, except that when I wrote this code, I wanted to add some error checking for the ACPI tables, and this is what I came up with.
If you're telling me that the code is wrong and it will generate false positives, then I can fix it. But if you're telling me that you don't understand why I'm doing some error checking on the ACPI tables that I'm parsing, well, I don't understand what could be wrong with that.
I understand the checking. I don't understand why you think you need an error message instead of just returning ENODEV if the tables are not there.
It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.
That doesn't explain if length is supposed to be 16 or 8 bit, if the length is supposed to be stored as big or little endian value, and what would happen if it was stored as big endian value on some system and is 16 bit wide.
All lengths and endianness of the fields in the ACPI structures are already defined in the ACPI spec, so that stuff is fixed and already known. I'm not sure what you're getting at. I'm just doing some basic error checking, no different than any of the code that calls BAD_MADT_ENTRY does.
In fact, I suspect that the only reason that BAD_GTDT_ENTRY does not yet exist is because there isn't much support for ACPI timers in the kernel yet.
If the length is in fact 16 bit, you could just check its value directly, instead of doing all the typecasting. And if it is 16 bit and has a fixed endianness (not host byte order), you should convert it to host byte order before validating it. Alternatively, something appears to be wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog.
I don't understand what you mean when you say something is wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog. These structures look perfectly fine to me, and they work. My driver successfully loads and parses the ACPI tables on a real ARM64 server system.
The length field is either 8 bit long or 16 bit. Your code assumes both.
I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.
ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?
Yes, ACPI for ARM64 *servers* is mandatory. ARM64 servers are not supposed to use device tree.
Question here is if enabling ACPI support disables devcietree, and if enabling devicetree disables ACPI. If both can be enabled, and if an image can be built which supports both, this would result in stray and unexpected error messages if the image is loaded on a system supporting devicetree.
If both ACPI and OF can be enabled, and if you want the error messages, I would expect to see an additional dependency on !OF.
And since this is a driver for SBSA systems (SBSA = Server Base System Architecture), this driver will only ever be used on an ARM64 server system with ACPI and no device tree at all.
Question though is if the driver can (and will) be built into an image which can run on other systems. There is no SBSA dependency, after all, only an ACPI dependency. If there is no "silent" means for the driver to fail instantiation, and if the driver is always loaded if present in an image, I would expect that it must only be built into an image if that image is built specifically to _only_ run on SBSA systems.
Assuming that there can be an image which boots both with ACPI and devicetree based configurations, I can understand that this driver would only load on ACPI based arm64 systems, but that doesn't mean it should dump an error message if the system does not use ACPI (or if its ACPI tables do not include an entry for the watchdog, for that matter).
I can have the driver silently exit if the GTDT table is missing. However, if it does exist, then all the other error messages are valid.
So ACPI based watchdog support and ACPI_SIG_GTDT / ACPI_GTDT_TYPE_WATCHDOG is mandatory for arm64 servers which support ACPI ?
Thanks, Guenter
On 05/01/2015 02:26 PM, Guenter Roeck wrote:
I can understand why that mandates that the driver has to be built into the kernel, at least if you and/or the arm64 maintainers don't want to export those functions. However, I still don't understand why you would need arm_sbsa_wdt_pdev declared twice.
What am I missing here ?
Sorry, I didn't completely read your email. I thought you were asking why it couldn't be compiled as a module.
The local version of arm_sbsa_wdt_pdev should be deleted. In fact, I could have sworn I fixed that bug already, but apparently not.
If you're telling me that the code is wrong and it will generate false positives, then I can fix it. But if you're telling me that you don't understand why I'm doing some error checking on the ACPI tables that I'm parsing, well, I don't understand what could be wrong with that.
I understand the checking. I don't understand why you think you need an error message instead of just returning ENODEV if the tables are not there.
So I've modified the code to not display a message if the tables are merely absent. It will still print an error if the table is invalid.
I don't understand what you mean when you say something is wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog. These structures look perfectly fine to me, and they work. My driver successfully loads and parses the ACPI tables on a real ARM64 server system.
The length field is either 8 bit long or 16 bit. Your code assumes both.
Ah, now I see it. I will have to look into that and get back to you.
Question here is if enabling ACPI support disables devcietree, and if enabling devicetree disables ACPI. If both can be enabled, and if an image can be built which supports both, this would result in stray and unexpected error messages if the image is loaded on a system supporting devicetree.
Enabling ACPI does not disable DT, because the EFI stub creates a "mini DT" that is still used by the kernel. I forgot what's in it, but I think the DT contains a pointer to the ACPI tables. However, no actual device information is in that DT.
If both ACPI and OF can be enabled, and if you want the error messages, I would expect to see an additional dependency on !OF.
Well, arm_sbsa_wdt_parse_gtdt() will only be called if the watchdog subtable in the GTDT exists.
Question though is if the driver can (and will) be built into an image which can run on other systems. There is no SBSA dependency, after all, only an ACPI dependency. If there is no "silent" means for the driver to fail instantiation, and if the driver is always loaded if present in an image, I would expect that it must only be built into an image if that image is built specifically to _only_ run on SBSA systems.
Yes, you're right. I've modified the driver so that it exits quietly if there is no GTDT or watchdog subtable.
So ACPI based watchdog support and ACPI_SIG_GTDT / ACPI_GTDT_TYPE_WATCHDOG is mandatory for arm64 servers which support ACPI ?
Probably not.
On 05/01/2015 02:24 PM, Arnd Bergmann wrote:
No, that is not a reasonable assumption to make. All the ARM64 server systems we have today are using DT only and we will of course keep supporting systems like that.
This is what drives me crazy about Linux kernel development. I've been doing this for 15 years, and I still get random emails that contradict everything I've been told to date.
I was repeatedly told over the past year by multiple people that ARM64 server == ACPI, no ifs, ands, or buts.
This means we need a DT binding for the driver, and the ACPI portion that creates the platform device should be split out from the main driver.
Well, someone who actually has a DT-based ARM64 server system (which is not me) should come up with such a definition.
I'm hoping that my driver can be accepted without DT support, and that someone else who is interested in running my driver on an SBSA device tree platform can add what's missing.
On 05/01/2015 02:19 PM, Arnd Bergmann wrote:
That should be fixed, we want normal drivers to be loadable modules.
The driver references three functions that are not exported:
ERROR: "acpi_parse_entries" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_get_rate" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_read_counter" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
I know what you're going to say. You want me to move the ACPI platform code into some ACPI platform file, maybe /arch/arm64/kernel/acpi.c. My concern is that there is currently no code for ACPI timers, so I don't have much of a starting point. I'd hate to have to define support for all of ACPI timers just to get my driver merged.
On 05/01/2015 01:15 PM, Timur Tabi wrote:
On 05/01/2015 02:19 PM, Arnd Bergmann wrote:
That should be fixed, we want normal drivers to be loadable modules.
The driver references three functions that are not exported:
ERROR: "acpi_parse_entries" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_get_rate" [drivers/watchdog/arm_sbsa_wdt.ko] undefined! ERROR: "arch_timer_read_counter" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
I know what you're going to say. You want me to move the ACPI platform code into some ACPI platform file, maybe /arch/arm64/kernel/acpi.c. My concern is that there is currently no code for ACPI timers, so I don't have much of a starting point. I'd hate to have to define support for all of ACPI timers just to get my driver merged.
Not really, at least not me. However, there is no other caller of arch_timer_get_rate, except for the call setting arch_timer_rate. This suggests that it might not be such a good idea to call arch_timer_get_rate() and arch_timer_read_counter() in the first place.
[ On a side note, arch_timer_get_rate() can return 0 if ARM_ARCH_TIMER is not configured. You'll need to check for that. ]
It is hard to imagine that the watchdog would be the only driver which needs this clock. Isn't there some clock API call that can be used instead ?
As for acpi_parse_entries, we will need some feedback from the acpi maintainers. If the intent is that this function can be called from drivers, it should be exported.
Guenter
Guenter Roeck wrote:
Not really, at least not me. However, there is no other caller of arch_timer_get_rate, except for the call setting arch_timer_rate. This suggests that it might not be such a good idea to call arch_timer_get_rate() and arch_timer_read_counter() in the first place.
I don't know of any other way to convert seconds into watchdog ticks.
[ On a side note, arch_timer_get_rate() can return 0 if ARM_ARCH_TIMER is not configured. You'll need to check for that. ]
Check the Kconfig for this driver. It requires ARM_ARCH_TIMER.
It is hard to imagine that the watchdog would be the only driver which needs this clock. Isn't there some clock API call that can be used instead ?
If there is, I'd love to know it. There's no 'device' for this driver, so I don't think the clk API will work.
As for acpi_parse_entries, we will need some feedback from the acpi maintainers. If the intent is that this function can be called from drivers, it should be exported.
Agreed.
On 05/01/2015 12:56 PM, Timur Tabi wrote:
On 05/01/2015 02:24 PM, Arnd Bergmann wrote:
No, that is not a reasonable assumption to make. All the ARM64 server systems we have today are using DT only and we will of course keep supporting systems like that.
This is what drives me crazy about Linux kernel development. I've been doing this for 15 years, and I still get random emails that contradict everything I've been told to date.
I was repeatedly told over the past year by multiple people that ARM64 server == ACPI, no ifs, ands, or buts.
I don't think anyone ever claimed that Linux development is free of politics. Except for master politicians, of course.
This means we need a DT binding for the driver, and the ACPI portion that creates the platform device should be split out from the main driver.
Well, someone who actually has a DT-based ARM64 server system (which is not me) should come up with such a definition.
I'm hoping that my driver can be accepted without DT support, and that someone else who is interested in running my driver on an SBSA device tree platform can add what's missing.
I am perfectly fine with that. All I am asking for is flexibility on your part.
Even if you (and/or others) may personally believe that ACPI _is_ the center of the universe, please be open to the possibility that others may have a different opinion and/or different requirements. We should not have to completely rewrite the driver to add devicetree support later on.
Note we still have to figure out how to instantiate the device based on the acpi data. I have no idea how that should be done, but it seems odd if it is really supposed to be done as you have implemented it. Maybe that is the case, but if so I would like to get a confirmation from an acpi maintainer.
Thanks, Guenter
On 05/01/2015 04:22 PM, Timur Tabi wrote:
Guenter Roeck wrote:
Not really, at least not me. However, there is no other caller of arch_timer_get_rate, except for the call setting arch_timer_rate. This suggests that it might not be such a good idea to call arch_timer_get_rate() and arch_timer_read_counter() in the first place.
I don't know of any other way to convert seconds into watchdog ticks.
[ On a side note, arch_timer_get_rate() can return 0 if ARM_ARCH_TIMER is not configured. You'll need to check for that. ]
Check the Kconfig for this driver. It requires ARM_ARCH_TIMER.
It is hard to imagine that the watchdog would be the only driver which needs this clock. Isn't there some clock API call that can be used instead ?
If there is, I'd love to know it. There's no 'device' for this driver, so I don't think the clk API will work.
Maybe ask on the arm mailing list ?
Guenter
As for acpi_parse_entries, we will need some feedback from the acpi maintainers. If the intent is that this function can be called from drivers, it should be exported.
Agreed.
Guenter Roeck wrote:
Even if you (and/or others) may personally believe that ACPI _is_ the center of the universe, please be open to the possibility that others may have a different opinion and/or different requirements. We should not have to completely rewrite the driver to add devicetree support later on.
There won't be any need to rewrite the driver to add device tree support. All we need to do is add the device tree probe function.
Note we still have to figure out how to instantiate the device based on the acpi data. I have no idea how that should be done, but it seems odd if it is really supposed to be done as you have implemented it. Maybe that is the case, but if so I would like to get a confirmation from an acpi maintainer.
Me too.
-
Arnd Bergmann -
Guenter Roeck -
Timur Tabi