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
| ||
|
Message-ID: <31925752-8028-98fc-b6a4-9b8fe8a3ce0b@ti.com> Date: Mon, 22 May 2023 14:00:04 -0500 From: Judith Mendez <jm@...com> To: Marc Kleine-Budde <mkl@...gutronix.de> CC: Chandrasekar Ramakrishnan <rcsekar@...sung.com>, <linux-can@...r.kernel.org>, Wolfgang Grandegger <wg@...ndegger.com>, "David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Schuyler Patton <spatton@...com>, Tero Kristo <kristo@...nel.org>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, <linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.kernel.org>, Oliver Hartkopp <socketcan@...tkopp.net>, Conor Dooley <conor+dt@...nel.org> Subject: Re: [PATCH v6 2/2] can: m_can: Add hrtimer to generate software interrupt Hello, On 5/22/23 1:37 PM, Marc Kleine-Budde wrote: > On 22.05.2023 10:17:38, Judith Mendez wrote: >>>> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c >>>> index 94dc82644113..3e60cebd9d12 100644 >>>> --- a/drivers/net/can/m_can/m_can_platform.c >>>> +++ b/drivers/net/can/m_can/m_can_platform.c >>>> @@ -5,6 +5,7 @@ >>>> // >>>> // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ >>>> +#include <linux/hrtimer.h> >>>> #include <linux/phy/phy.h> >>>> #include <linux/platform_device.h> >>>> @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) >>>> goto probe_fail; >>>> addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); >>>> - irq = platform_get_irq_byname(pdev, "int0"); >>>> - if (IS_ERR(addr) || irq < 0) { >>>> - ret = -EINVAL; >>>> + if (IS_ERR(addr)) { >>>> + ret = PTR_ERR(addr); >>>> goto probe_fail; >>>> } >>> >>> As we don't use an explicit "poll-interval" anymore, this needs some >>> cleanup. The flow should be (pseudo code, error handling omitted): >>> >>> if (device_property_present("interrupts") { >>> platform_get_irq_byname(); >>> polling = false; >>> } else { >>> hrtimer_init(); >>> polling = true; >>> } >> >> Ok. >> >>> >>>> + irq = platform_get_irq_byname_optional(pdev, "int0"); >>> >>> Remove the "_optional" and.... >> >> On V2, you asked to add the _optional?..... >> >>> irq = platform_get_irq_byname(pdev, "int0"); >> >> use platform_get_irq_byname_optional(), it doesn't print an error >> message. > > ACK - I said that back in v2, when there was "poll-interval". But now we > don't use "poll-interval" anymore, but test if interrupt properties are > present. > > See again pseudo-code I posted in my last mail: > > | if (device_property_present("interrupts") { > | platform_get_irq_byname(); > > If this throws an error, it's fatal, bail out. > > | polling = false; > | } else { > | hrtimer_init(); > | polling = true; > | } > Ok, will add this then.. >> >>> >>>> + if (irq == -EPROBE_DEFER) { >>>> + ret = -EPROBE_DEFER; >>>> + goto probe_fail; >>>> + } >>>> + >>>> + if (device_property_present(mcan_class->dev, "interrupts") || >>>> + device_property_present(mcan_class->dev, "interrupt-names")) >>>> + mcan_class->polling = false; >>> >>> ...move the platform_get_irq_byname() here >> >> ok, >> >>> >>>> + else >>>> + mcan_class->polling = true; >>>> + >>>> + if (!mcan_class->polling && irq < 0) { >>>> + ret = -ENXIO; >>>> + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); >>>> + goto probe_fail; >>>> + } >>> >>> Remove this check. >> >> Should we not go to 'probe fail' if polling is not activated and irq is not >> found? > > If an interrupt property is present in the DT, we use it - if request > IRQ fails, something is broken and we've already bailed out. See above. > If there is no interrupt property we use polling. Got it, thanks. >> >>> >>>> + >>>> + if (mcan_class->polling) { >>>> + if (irq > 0) { >>>> + mcan_class->polling = false; >>>> + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); >>> >>> Remove this. >> >> Remove the dev_info? > > ACK, this is not possible anymore - we cannot have polling enabled and > HW IRQs configured. Sounds good, will submit a v7 with these cleanup changes. regards, Judith
Powered by blists - more mailing lists