| 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: <AM4PR0401MB22606C1B2E8CBF3DDF9F4D88FF420@AM4PR0401MB2260.eurprd04.prod.outlook.com>
Date: Thu, 19 Oct 2017 06:25:39 +0000
From: Andy Duan <fugang.duan@....com>
To: Troy Kisky <troy.kisky@...ndarydevices.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
From: Troy Kisky <troy.kisky@...ndarydevices.com> Sent: Thursday, October 19, 2017 2:30 AM
>On 10/18/2017 11:10 AM, Troy Kisky wrote:
>> 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.
>
>
>Whoops I meant "ptp" clock.
>
>That brings up a question though.
>
>interrupt-names = "int0", "int1", "int2", "pps"
>
>may be more accurate. Is there any desire for me to use "pps" instead ?
>
I agree this named method.
Set the property "interrupt-names" is optional properties that don't break other platforms.
Powered by blists - more mailing lists