| 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
| ||
|
Date: Wed, 18 Oct 2017 11:10:52 -0700
From: Troy Kisky <troy.kisky@...ndarydevices.com>
To: Andy Duan <fugang.duan@....com>,
"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>
Cc: Fabio Estevam <fabio.estevam@....com>,
"lznuaa@...il.com" <lznuaa@...il.com>,
"andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt
routine
On 10/17/2017 7:30 PM, Andy Duan wrote:
> From: Troy Kisky <troy.kisky@...ndarydevices.com> Sent: Wednesday, October 18, 2017 5:34 AM
>>>> This is better for code locality and should slightly speed up normal
>> interrupts.
>>>>
>>>> This also allows PPS clock output to start working for i.mx7. This is
>>>> because
>>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>>
>>>> Signed-off-by: Troy Kisky <troy.kisky@...ndarydevices.com>
>>>>
>>>> ---
>>>>
>>>> v2: made this change independent of any devicetree change so that old
>>>> dtbs continue to work.
>>>>
>>>> Continue to register ptp clock if interrupt is not found.
>>>> ---
>>>> drivers/net/ethernet/freescale/fec.h | 3 +-
>>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>>> drivers/net/ethernet/freescale/fec_ptp.c | 82
>>>> ++++++++++++++++++--------
>>>> -----
>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>>> b/drivers/net/ethernet/freescale/fec.h
>>>> index ede1876a9a19..be56ac1f1ac4 100644
>>>> --- a/drivers/net/ethernet/freescale/fec.h
>>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>>> u64 ethtool_stats[0];
>>>> };
>>>>
>>>> -void fec_ptp_init(struct platform_device *pdev);
>>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>>> void fec_ptp_stop(struct platform_device *pdev); void
>>>> fec_ptp_start_cyclecounter(struct net_device *ndev); int
>>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr); int
>>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>>
>>>>
>>>>
>> /**********************************************************
>>>> ******************/
>>>> #endif /* FEC_H */
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>> index 3dc2d771a222..21afabbc560f 100644
>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>>> ret = IRQ_HANDLED;
>>>> complete(&fep->mdio_done);
>>>> }
>>>> -
>>>> - if (fep->ptp_clock)
>>>> - if (fec_ptp_check_pps_event(fep))
>>>> - ret = IRQ_HANDLED;
>>>> return ret;
>>>> }
>>>>
>>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>>> struct device_node *np = pdev->dev.of_node, *phy_node;
>>>> int num_tx_qs;
>>>> int num_rx_qs;
>>>> + char irq_name[8];
>>>> + int irq_cnt;
>>>>
>>>> fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>>
>>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>>> if (ret)
>>>> goto failed_reset;
>>>>
>>>> + irq_cnt = platform_irq_count(pdev);
>>>> + if (irq_cnt > FEC_IRQ_NUM)
>>>> + irq_cnt = FEC_IRQ_NUM; /* last for ptp */
>>>> + else if (irq_cnt == 2)
>>>> + irq_cnt = 1; /* last for ptp */
>>>> + else if (irq_cnt <= 0)
>>>> + irq_cnt = 1; /* Let the for loop fail */
>>>
>>> Don't do like this. Don't suppose pps interrupt is the last one.
>>
>>
>> I don't. If the pps interrupt is named, the named interrupt will be used. If it is
>> NOT named, the last interrupt is used, if 2 interrupts, or >3 interrupt are
>> provided.
>> Otherwise, no pps interrupt is assumed.
>> Fortunately this seems to be true currently.
>>
> If pps interrupt is not named, then it limit the last one is pps.
> We cannot get the pps interrupt based on current chip interrupt define, we never know the future chip how to define interrupt.
> Although your current implementation can work with current chips, but it is not really good solution.
>
>>
>>> And if irq_cnt is 1 like imx28/imx5x, the patch will break fec interrupt
>> function.
>>
>> How ? fec_ptp_init will not be called as bufdesc_ex is 0.
>>
> Imx28 also support enhanced buffer descriptor, if define the ptp clock in dts then bufdesc_ex also can be 1.
Only if FEC_QUIRK_HAS_BUFDESC_EX is set, which it is not. Here's the relevant code snippets
}, {
.name = "imx25-fec",
.driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_MIB_CLEAR,
}, {
.name = "imx27-fec",
.driver_data = FEC_QUIRK_MIB_CLEAR,
}, {
.name = "imx28-fec",
.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
FEC_QUIRK_SINGLE_MDIO | FEC_QUIRK_HAS_RACC,
}, {
....
fep->bufdesc_ex = fep->quirks & FEC_QUIRK_HAS_BUFDESC_EX;
fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
if (IS_ERR(fep->clk_ptp)) {
fep->clk_ptp = NULL;
fep->bufdesc_ex = false;
}
______________
You could make your way work though, if you remove the pps clock from imx50.dtsi, imx51.dtsi, and
imx53.dtsi.
Then you would not need the FEC_QUIRK_HAS_BUFDESC_EX quirk.
This is all a little off topic though.
>
> I still suggest to use v1 logic check pps interrupt that need to check irq name.
I could add that back. But it seems redundant when also looking up by name "int0", "int1", "int2".
I would think that we could recommend in the binding documentation that either all are
named, or none are. And I don't think we need to make work mistakes like
interrupt-names = "irq0", "ptp", "int1", "int2"
but
interrupt-names = "int0", "ptp", "int1", "int2"
should currently work.
Let me know if you want me to make work
interrupt-names = "", "ptp", "", ""
or if you just don't think this patch is necessary. I might have some more
reasons to try and persuade you.
>
>>
>> Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps
>> interrupt.
>>
>>
>>>
>>> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like
>> your v1 logic check.
>>>
>>>> +
>>>> if (fep->bufdesc_ex)
>>>> - fec_ptp_init(pdev);
>>>> + fec_ptp_init(pdev, irq_cnt);
>>>>
>>>> ret = fec_enet_init(ndev);
>>>> if (ret)
>>>> goto failed_init;
>>>>
>>>> - for (i = 0; i < FEC_IRQ_NUM; i++) {
>>>> - irq = platform_get_irq(pdev, i);
>>>> + for (i = 0; i < irq_cnt; i++) {
>>>> + sprintf(irq_name, "int%d", i);
>>>> + irq = platform_get_irq_byname(pdev, irq_name);
>>>> + if (irq < 0)
>>>> + irq = platform_get_irq(pdev, i);
>>>> if (irq < 0) {
>>>> - if (i)
>>>> - break;
>>>> ret = irq;
>>>> goto failed_irq;
>>>> }
Thanks for the review
Troy
Powered by blists - more mailing lists