| 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: <CAH1o70LnEEu3iYos8wsaEZ=T67N+Tex_XHZgE9P93n6X6Bzz4A@mail.gmail.com>
Date: Fri, 18 Dec 2015 01:29:44 +0900
From: Yoshihiro Kaneko <ykaneko0929@...il.com>
To: Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Simon Horman <horms@...ge.net.au>,
Magnus Damm <magnus.damm@...il.com>,
Linux-sh list <linux-sh@...r.kernel.org>
Subject: Re: [PATCH/RFC net-next] ravb: Add dma queue interrupt support
Hi,
2015-12-16 4:00 GMT+09:00 Sergei Shtylyov <sergei.shtylyov@...entembedded.com>:
> Hello.
>
> On 12/15/2015 03:23 PM, Yoshihiro Kaneko wrote:
>
>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@...esas.com>
>>
>> This patch supports the following interrupts.
>>
>> - One interrupt for multiple (descriptor, error, management)
>> - One interrupt for emac
>> - Four interrupts for dma queue (best effort rx/tx, network control rx/tx)
>
>
> You don't say why the current 2-interrupt scheme (implemented by Simon's
> patch) isn't enpough...
>
>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@...esas.com>
>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@...il.com>
>
> [...]
>
>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>> b/drivers/net/ethernet/renesas/ravb.h
>> index 9fbe92a..eada5a1 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -157,6 +157,7 @@ enum ravb_reg {
>> TIC = 0x0378,
>> TIS = 0x037C,
>> ISS = 0x0380,
>> + CIE = 0x0384,
>
>
> I'd like to see some comment clarifying that this is R-Car gen3 only reg.
>
> [./..]
>>
>> @@ -556,6 +566,16 @@ enum ISS_BIT {
>> ISS_DPS15 = 0x80000000,
>> };
>>
>> +/* CIE */
>
>
> And here as well.
>
>> +enum CIE_BIT {
>> + CIE_CRIE = 0x00000001, /* Common Receive Interrupt Enable
>> */
>> + CIE_CTIE = 0x00000100, /* Common Transmit Interrupt Enable
>> */
>> + CIE_RQFM = 0x00010000, /* Reception Queue Full Mode */
>> + CIE_CL0M = 0x00020000, /* Common Line 0 Mode */
>> + CIE_RFWL = 0x00040000, /* Rx-FIFO Warning interrupt Line */
>
>
> You forgot "Select" at the end.
>
>> + CIE_RFFL = 0x00080000, /* Rx-FIFO Full interrupt Line */
>
>
> Here as well.
> Well, generally we don't have such comments for the other registers, so
> this will look somewhat out of line...
I agree. I will remove those comment.
>
> [...]
>>
>> @@ -592,6 +612,18 @@ enum GIS_BIT {
>> GIS_PTMF = 0x00000004,
>> };
>>
>> +/* GIx */
>
>
> I'd prefer GIC/GIS.
>
>> +#define RAVB_GIx_ALL 0xffff03ff
>
>
> No RAVB_ prefix please.
>
>> +
>> +/* RIx0 */
>
>
> RIE0/RID0.
>
>> +#define RAVB_RIx0_ALL 0x0003ffff
>
>
> No prefix. And I'd rather call it RIx0_FRx. Or even RIE0_FRS and
> RID0_FRD.
>
>> +
>> +/* RIx2 */
>
>
> RIE2/RID2.
>
>> +#define RAVB_RIx2_ALL 0x8003ffff
>
>
> No prefix. And there's bit 31 in this register, according to my gen3
> manual. So, your _ALL isn't really "all bits". I'd rather call it RIx2_QFx.
> Or even RIE2_QFS and RID2_QFD.
I think that bit 31 is included in the value 0x8003ffff. Or I'm
missing something?
>
>> +
>> +/* TIx */
>
>
> TIE/TID.
>
>> +#define RAVB_TIx_ALL 0x000fffff
>
>
> No prefix. And there's bit 31 in this register, according to my gen3
> manual. So, your _ALL isn't really "all bits".
I think the correct value is 0x000f0f0f.
>
> [...]
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 120cc25..753b67d 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>
> [...]
>>
>> @@ -376,6 +386,7 @@ static void ravb_emac_init(struct net_device *ndev)
>> static int ravb_dmac_init(struct net_device *ndev)
>> {
>> int error;
>> + struct ravb_private *priv = netdev_priv(ndev);
>
>
> Please declare this variable before 'error' -- DaveM really prefers
> "reversed Christmas tree" declaration order.
>
> [...]
>>
>> @@ -411,14 +422,28 @@ static int ravb_dmac_init(struct net_device *ndev)
>> ravb_write(ndev, TCCR_TFEN, TCCR);
>>
>> /* Interrupt init: */
>> - /* Frame receive */
>> - ravb_write(ndev, RIC0_FRE0 | RIC0_FRE1, RIC0);
>> - /* Disable FIFO full warning */
>> - ravb_write(ndev, 0, RIC1);
>> - /* Receive FIFO full error, descriptor empty */
>> - ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
>> - /* Frame transmitted, timestamp FIFO updated */
>> - ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
>> + if (priv->chip_id == RCAR_GEN2) {
>> + /* Frame receive */
>> + ravb_write(ndev, RIC0_FRE0 | RIC0_FRE1, RIC0);
>> + /* Disable FIFO full warning */
>> + ravb_write(ndev, 0, RIC1);
>> + /* Receive FIFO full error, descriptor empty */
>> + ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
>> + /* Frame transmitted, timestamp FIFO updated */
>> + ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
>> + } else {
>> + /* Clear CIE.CTIE, CIE.CRIE, DIL.DPLx */
>> + ravb_write(ndev, 0, CIE);
>
>
> Why clear CIE if you immediately overwrite it?
>
>> + ravb_write(ndev, 0, DIL);
>> + /* Set queue specific interrupt */
>> + ravb_write(ndev, CIE_CRIE | CIE_CTIE | CIE_CL0M, CIE);
>> + /* Frame receive */
>> + ravb_write(ndev, RIC0_FRE0 | RIC0_FRE1, RIE0);
>
>
> Using RIC0 bits to write to RIE0?
>
>> + /* Receive FIFO full error, descriptor empty */
>> + ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIE2);
>
>
> Using RIC2 bits to write to RIE2?
>
>> + /* Frame transmitted, timestamp FIFO updated */
>> + ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIE);
>
>
> Using TIC bits to write to TIE?
> No, that won't do.
>
>> @@ -654,7 +679,7 @@ static int ravb_stop_dma(struct net_device *ndev)
>> }
>>
>> /* E-MAC interrupt handler */
>> -static void ravb_emac_interrupt(struct net_device *ndev)
>> +static void _ravb_emac_interrupt(struct net_device *ndev)
>
>
> ravb_emac_interrupt_[un]locked() perhaps? Not sure which is more
> correct... :-)
How about ravb_process_emac_interrupt() ?
>
> [...]
>>
>> @@ -690,7 +727,10 @@ static void ravb_error_interrupt(struct net_device
>> *ndev)
>> ravb_write(ndev, ~EIS_QFS, EIS);
>> if (eis & EIS_QFS) {
>> ris2 = ravb_read(ndev, RIS2);
>> - ravb_write(ndev, ~(RIS2_QFF0 | RIS2_RFFF), RIS2);
>> + if (priv->chip_id == RCAR_GEN2)
>> + ravb_write(ndev, ~(RIS2_QFF0 | RIS2_RFFF), RIS2);
>> + else
>> + ravb_write(ndev, RIS2_QFF0 | RIS2_RFFF, RID2);
>
>
> Using RIS2 bits to write to RID2? That won't do.
>
> [...]
>>
>> @@ -758,16 +798,43 @@ static irqreturn_t ravb_interrupt(int irq, void
>> *dev_id)
>
> [...]
>>
>> +/* Descriptor IRQ/ Error/ Management interrupt handler */
>
>
> No space after /.
>
>> +static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
>> +{
>> + struct net_device *ndev = dev_id;
>> + struct ravb_private *priv = netdev_priv(ndev);
>> + irqreturn_t result = IRQ_NONE;
>> + u32 iss;
>> +
>> + spin_lock(&priv->lock);
>> + /* Get interrupt status */
>> + iss = ravb_read(ndev, ISS);
>> +
>> /* Error status summary */
>> if (iss & ISS_ES) {
>> ravb_error_interrupt(ndev);
>> result = IRQ_HANDLED;
>> }
>>
>> + /* Management */
>
>
> Really? I thought that's gPTP Interrupt...
gPTP seems to be a part of Management related interrupts.
>
>> if (iss & ISS_CGIS)
>> result = ravb_ptp_interrupt(ndev);
>>
>> @@ -776,6 +843,55 @@ static irqreturn_t ravb_interrupt(int irq, void
>> *dev_id)
>
> [...]
>>
>> @@ -815,8 +931,13 @@ static int ravb_poll(struct napi_struct *napi, int
>> budget)
>>
>> /* Re-enable RX/TX interrupts */
>> spin_lock_irqsave(&priv->lock, flags);
>> - ravb_write(ndev, ravb_read(ndev, RIC0) | mask, RIC0);
>> - ravb_write(ndev, ravb_read(ndev, TIC) | mask, TIC);
>> + if (priv->chip_id == RCAR_GEN2) {
>> + ravb_write(ndev, ravb_read(ndev, RIC0) | mask, RIC0);
>> + ravb_write(ndev, ravb_read(ndev, TIC) | mask, TIC);
>
>
> You can drop one space before TIC now, sorry for that. :-)
>
>> + } else {
>> + ravb_write(ndev, mask, RIE0);
>> + ravb_write(ndev, mask, TIE);
>> + }
>> mmiowb();
>> spin_unlock_irqrestore(&priv->lock, flags);
>>
>> @@ -1208,23 +1329,68 @@ static const struct ethtool_ops ravb_ethtool_ops =
>> {
>> static int ravb_open(struct net_device *ndev)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> - int error;
>> + int error, i;
>> + struct platform_device *pdev = priv->pdev;
>> + struct device *dev = &pdev->dev;
>> + char *name;
>
>
> Reverse Christmas tree, please. :-)
>
> [...]
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c
>> b/drivers/net/ethernet/renesas/ravb_ptp.c
>> index 7a8ce92..a789c7c 100644
>> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
>> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
>> @@ -196,11 +196,15 @@ static int ravb_ptp_extts(struct ptp_clock_info
>> *ptp,
>>
>> spin_lock_irqsave(&priv->lock, flags);
>> gic = ravb_read(ndev, GIC);
>> - if (on)
>> - gic |= GIC_PTCE;
>> - else
>> - gic &= ~GIC_PTCE;
>> - ravb_write(ndev, gic, GIC);
>> + if (priv->chip_id == RCAR_GEN2) {
>> + if (on)
>> + gic |= GIC_PTCE;
>> + else
>> + gic &= ~GIC_PTCE;
>> + ravb_write(ndev, gic, GIC);
>> + } else {
>> + ravb_write(ndev, GIC_PTCE, (on) ? GIE : GID);
>
>
> Using GIC bit to write GIE/GID? Won't do.
>
> [...]
>>
>> @@ -248,9 +252,13 @@ static int ravb_ptp_perout(struct ptp_clock_info
>> *ptp,
>> error = ravb_ptp_update_compare(priv, (u32)start_ns);
>> if (!error) {
>> /* Unmask interrupt */
>> - gic = ravb_read(ndev, GIC);
>> - gic |= GIC_PTME;
>> - ravb_write(ndev, gic, GIC);
>> + if (priv->chip_id == RCAR_GEN2) {
>> + gic = ravb_read(ndev, GIC);
>> + gic |= GIC_PTME;
>> + ravb_write(ndev, gic, GIC);
>> + } else {
>> + ravb_write(ndev, GIC_PTME, GIE);
>> + }
>
>
> Again?
>
> [...]
>>
>> @@ -259,9 +267,13 @@ static int ravb_ptp_perout(struct ptp_clock_info
>> *ptp,
>> perout->period = 0;
>>
>> /* Mask interrupt */
>> - gic = ravb_read(ndev, GIC);
>> - gic &= ~GIC_PTME;
>> - ravb_write(ndev, gic, GIC);
>> + if (priv->chip_id == RCAR_GEN2) {
>> + gic = ravb_read(ndev, GIC);
>> + gic &= ~GIC_PTME;
>> + ravb_write(ndev, gic, GIC);
>> + } else {
>> + ravb_write(ndev, GIC_PTME, GID);
>
>
> Again?
> No way.
>
> [...]
>
> MBR, Sergei
>
Thanks,
kaneko
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists