* [PATCH net] net: ethernet: stmmac: Fix signedness bug in ipq806x_gmac_of_parse()
From: Dan Carpenter @ 2019-09-25 11:05 UTC (permalink / raw)
To: Giuseppe Cavallaro, Mathieu Olivari
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Maxime Coquelin,
netdev, linux-stm32, kernel-janitors
The "gmac->phy_mode" variable is an enum and in this context GCC will
treat it as an unsigned int so the error handling will never be
triggered.
Fixes: b1c17215d718 ("stmmac: add ipq806x glue layer")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
index 2c6d7c69c8f7..0d21082ceb93 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
@@ -191,7 +191,7 @@ static int ipq806x_gmac_of_parse(struct ipq806x_gmac *gmac)
struct device *dev = &gmac->pdev->dev;
gmac->phy_mode = of_get_phy_mode(dev->of_node);
- if (gmac->phy_mode < 0) {
+ if ((int)gmac->phy_mode < 0) {
dev_err(dev, "missing phy mode property\n");
return -EINVAL;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net] net: axienet: fix a signedness bug in probe
From: Alvaro G. M @ 2019-09-25 11:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Radhey Shyam Pandey, David S. Miller, Michal Simek, Russell King,
netdev, kernel-janitors
In-Reply-To: <20190925105911.GI3264@mwanda>
Hi, Dan
On Wed, Sep 25, 2019 at 01:59:11PM +0300, Dan Carpenter wrote:
> The "lp->phy_mode" is an enum but in this context GCC treats it as an
> unsigned int so the error handling is never triggered.
>
> lp->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> - if (lp->phy_mode < 0) {
> + if ((int)lp->phy_mode < 0) {
This (almost) exact code appears in a lot of different drivers too,
so maybe it'd be nice to review them all and apply the same cast if needed?
Best regards
--
Alvaro G. M.
^ permalink raw reply
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: Thierry Reding @ 2019-09-25 11:17 UTC (permalink / raw)
To: David Miller
Cc: peppe.cavallaro, alexandre.torgue, joabreu, f.fainelli, jonathanh,
bbiswas, netdev, linux-tegra
In-Reply-To: <20190924.214508.1949579574079200671.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]
On Tue, Sep 24, 2019 at 09:45:08PM +0200, David Miller wrote:
> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Fri, 20 Sep 2019 19:00:34 +0200
>
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The DWMAC 4.10 supports the same enhanced addressing mode as later
> > generations. Parse this capability from the hardware feature registers
> > and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.
>
> This looks like an enhancement and/or optimization rather than a bug fix.
>
> Also, you're now writing to the high 32-bits unconditionally, even when
> it will always be zero because of 32-bit addressing. That looks like
> a step backwards to me.
>
> I'm not applying this.
Sounds like you would prefer v2 of this:
https://patchwork.ozlabs.org/project/netdev/list/?series=129768&state=*
While v2 didn't have a cover letter, it did write the upper 32 bits
conditionally.
Do you want to pick that up instead, or do you want me to send out a v4
with the cover letter from v3 and the patches from v2?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: David Miller @ 2019-09-25 11:33 UTC (permalink / raw)
To: Jose.Abreu
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
In-Reply-To: <BN8PR12MB3266F851B071629898BB775AD3870@BN8PR12MB3266.namprd12.prod.outlook.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Wed, 25 Sep 2019 10:44:53 +0000
> From: David Miller <davem@davemloft.net>
> Date: Sep/24/2019, 20:45:08 (UTC+00:00)
>
>> From: Thierry Reding <thierry.reding@gmail.com>
>> Date: Fri, 20 Sep 2019 19:00:34 +0200
>>
>> Also, you're now writing to the high 32-bits unconditionally, even when
>> it will always be zero because of 32-bit addressing. That looks like
>> a step backwards to me.
>
> Don't agree. As per previous discussions and as per my IP knowledge, if
> EAME is not enabled / not supported the register can still be written.
> This is not fast path and will not impact any remaining operation. Can
> you please explain what exactly is the concern about this ?
>
> Anyway, this is an important feature for performance so I hope Thierry
> re-submits this once -next opens and addressing the review comments.
Perhaps I misunderstand the context, isn't this code writing the
descriptors for every packet?
^ permalink raw reply
* Re: [PATCH net] net: axienet: fix a signedness bug in probe
From: David Miller @ 2019-09-25 11:35 UTC (permalink / raw)
To: alvaro.gamez
Cc: dan.carpenter, radhey.shyam.pandey, michal.simek, linux, netdev,
kernel-janitors
In-Reply-To: <20190925110542.GA21923@salem.gmr.ssr.upm.es>
From: "Alvaro G. M" <alvaro.gamez@hazent.com>
Date: Wed, 25 Sep 2019 13:05:43 +0200
> Hi, Dan
>
> On Wed, Sep 25, 2019 at 01:59:11PM +0300, Dan Carpenter wrote:
>> The "lp->phy_mode" is an enum but in this context GCC treats it as an
>> unsigned int so the error handling is never triggered.
>>
>> lp->phy_mode = of_get_phy_mode(pdev->dev.of_node);
>> - if (lp->phy_mode < 0) {
>> + if ((int)lp->phy_mode < 0) {
>
> This (almost) exact code appears in a lot of different drivers too,
> so maybe it'd be nice to review them all and apply the same cast if needed?
Or make the thing an int if negative values are never valid 32-bit phy_mode
values anyways.
^ permalink raw reply
* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: Jose Abreu @ 2019-09-25 11:41 UTC (permalink / raw)
To: David Miller, Jose.Abreu@synopsys.com
Cc: thierry.reding@gmail.com, peppe.cavallaro@st.com,
alexandre.torgue@st.com, f.fainelli@gmail.com,
jonathanh@nvidia.com, bbiswas@nvidia.com, netdev@vger.kernel.org,
linux-tegra@vger.kernel.org
In-Reply-To: <20190925.133353.1445361137776125638.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Sep/25/2019, 12:33:53 (UTC+00:00)
> From: Jose Abreu <Jose.Abreu@synopsys.com>
> Date: Wed, 25 Sep 2019 10:44:53 +0000
>
> > From: David Miller <davem@davemloft.net>
> > Date: Sep/24/2019, 20:45:08 (UTC+00:00)
> >
> >> From: Thierry Reding <thierry.reding@gmail.com>
> >> Date: Fri, 20 Sep 2019 19:00:34 +0200
> >>
> >> Also, you're now writing to the high 32-bits unconditionally, even when
> >> it will always be zero because of 32-bit addressing. That looks like
> >> a step backwards to me.
> >
> > Don't agree. As per previous discussions and as per my IP knowledge, if
> > EAME is not enabled / not supported the register can still be written.
> > This is not fast path and will not impact any remaining operation. Can
> > you please explain what exactly is the concern about this ?
> >
> > Anyway, this is an important feature for performance so I hope Thierry
> > re-submits this once -next opens and addressing the review comments.
>
> Perhaps I misunderstand the context, isn't this code writing the
> descriptors for every packet?
No, its just setting up the base address for the descriptors which is
done in open(). The one that's in the fast path is the tail address,
which is always the lower 32 bits.
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH v4 0/2] net/ibmvnic: serialization fixes
From: David Miller @ 2019-09-25 11:41 UTC (permalink / raw)
To: julietk; +Cc: netdev, tlfalcon, linuxppc-dev
In-Reply-To: <20190920201123.18913-1-julietk@linux.vnet.ibm.com>
From: Juliet Kim <julietk@linux.vnet.ibm.com>
Date: Fri, 20 Sep 2019 16:11:21 -0400
> This series includes two fixes. The first improves reset code to allow
> linkwatch_event to proceed during reset. The second ensures that no more
> than one thread runs in reset at a time.
>
> v2:
> - Separate change param reset from do_reset()
> - Return IBMVNIC_OPEN_FAILED if __ibmvnic_open fails
> - Remove setting wait_for_reset to false from __ibmvnic_reset(), this
> is done in wait_for_reset()
> - Move the check for force_reset_recovery from patch 1 to patch 2
>
> v3:
> - Restore reset’s successful return in open failure case
>
> v4:
> - Change resetting flag access to atomic
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH] hdlc: Simplify code in 'pvc_xmit()'
From: David Miller @ 2019-09-25 11:43 UTC (permalink / raw)
To: christophe.jaillet; +Cc: khc, netdev, linux-kernel, kernel-janitors
In-Reply-To: <20190921061738.25326-1-christophe.jaillet@wanadoo.fr>
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Date: Sat, 21 Sep 2019 08:17:38 +0200
> Use __skb_pad instead of rewriting it, this saves some LoC.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.
Please test this code and resubmit for net-next, thank you.
^ permalink raw reply
* Re: [PATCH] net: dsa: microchip: Always set regmap stride to 1
From: David Miller @ 2019-09-25 11:44 UTC (permalink / raw)
To: marex
Cc: netdev, andrew, f.fainelli, george.mccollister, Tristram.Ha,
vivien.didelot, woojung.huh
In-Reply-To: <20190921175309.2195-1-marex@denx.de>
From: Marek Vasut <marex@denx.de>
Date: Sat, 21 Sep 2019 19:53:09 +0200
> The regmap stride is set to 1 for regmap describing 8bit registers already.
> However, for 16/32/64bit registers, the stride is 2/4/8 respectively. This
> is not correct, as the switch protocol supports unaligned register reads
> and writes and the KSZ87xx even uses such unaligned register accesses to
> read e.g. MIB counter.
>
> This patch fixes MIB counter access on KSZ87xx.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Please resubmit with an appropriate Fixes: tag as per Florian's feedback.
Thank you.
^ permalink raw reply
* Re: [PATCH net] ppp: Fix memory leak in ppp_write
From: David Miller @ 2019-09-25 11:45 UTC (permalink / raw)
To: jeliantsurux; +Cc: gnault, paulus, netdev
In-Reply-To: <20190922074531.GA1450@DESKTOP>
From: Takeshi Misawa <jeliantsurux@gmail.com>
Date: Sun, 22 Sep 2019 16:45:31 +0900
> When ppp is closing, __ppp_xmit_process() failed to enqueue skb
> and skb allocated in ppp_write() is leaked.
>
> syzbot reported :
...
> Fix this by freeing skb, if ppp is closing.
>
> Fixes: 6d066734e9f0 ("ppp: avoid loop in xmit recursion detection code")
> Reported-and-tested-by: syzbot+d9c8bf24e56416d7ce2c@syzkaller.appspotmail.com
> Signed-off-by: Takeshi Misawa <jeliantsurux@gmail.com>
Applied and queued up for -stable.
^ permalink raw reply
* [PATCH] can: m_can: add support for one shot mode
From: Pankaj Sharma @ 2019-09-25 11:45 UTC (permalink / raw)
To: linux-can, netdev, linux-kernel
Cc: wg, mkl, davem, eugen.hristev, ludovic.desroches, pankaj.dubey,
rcsekar, Pankaj Sharma, Sriram Dash
In-Reply-To: <CGME20190925114609epcas5p305e259619c7fe8cdc75d9fd27f34e758@epcas5p3.samsung.com>
According to the CAN Specification (see ISO 11898-1:2015, 8.3.4
Recovery Management), the M_CAN provides means for automatic
retransmission of frames that have lost arbitration or that
have been disturbed by errors during transmission. By default
automatic retransmission is enabled.
The Bosch MCAN controller has support for disabling automatic
retransmission.
To support time-triggered communication as described in ISO
11898-1:2015, chapter 9.2, the automatic retransmission may be
disabled via CCCR.DAR.
CAN_CTRLMODE_ONE_SHOT is used for disabling automatic retransmission.
Signed-off-by: Pankaj Sharma <pankj.sharma@samsung.com>
Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
---
drivers/net/can/m_can/m_can.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index deb274a19ba0..e7165404ba8a 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -150,6 +150,7 @@ enum m_can_mram_cfg {
#define CCCR_CME_CANFD_BRS 0x2
#define CCCR_TXP BIT(14)
#define CCCR_TEST BIT(7)
+#define CCCR_DAR BIT(6)
#define CCCR_MON BIT(5)
#define CCCR_CSR BIT(4)
#define CCCR_CSA BIT(3)
@@ -1123,7 +1124,7 @@ static void m_can_chip_config(struct net_device *dev)
if (priv->version == 30) {
/* Version 3.0.x */
- cccr &= ~(CCCR_TEST | CCCR_MON |
+ cccr &= ~(CCCR_TEST | CCCR_MON | CCCR_DAR |
(CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
(CCCR_CME_MASK << CCCR_CME_SHIFT));
@@ -1133,7 +1134,7 @@ static void m_can_chip_config(struct net_device *dev)
} else {
/* Version 3.1.x or 3.2.x */
cccr &= ~(CCCR_TEST | CCCR_MON | CCCR_BRSE | CCCR_FDOE |
- CCCR_NISO);
+ CCCR_NISO | CCCR_DAR);
/* Only 3.2.x has NISO Bit implemented */
if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
@@ -1153,6 +1154,10 @@ static void m_can_chip_config(struct net_device *dev)
if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
cccr |= CCCR_MON;
+ /* Disable Auto Retransmission (all versions) */
+ if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+ cccr |= CCCR_DAR;
+
/* Write config */
m_can_write(priv, M_CAN_CCCR, cccr);
m_can_write(priv, M_CAN_TEST, test);
@@ -1291,7 +1296,8 @@ static int m_can_dev_setup(struct platform_device *pdev, struct net_device *dev,
priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
CAN_CTRLMODE_LISTENONLY |
CAN_CTRLMODE_BERR_REPORTING |
- CAN_CTRLMODE_FD;
+ CAN_CTRLMODE_FD |
+ CAN_CTRLMODE_ONE_SHOT;
/* Set properties depending on M_CAN version */
switch (priv->version) {
--
2.17.1
^ permalink raw reply related
* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: Jose Abreu @ 2019-09-25 11:46 UTC (permalink / raw)
To: David Miller, Jose.Abreu@synopsys.com
Cc: thierry.reding@gmail.com, peppe.cavallaro@st.com,
alexandre.torgue@st.com, f.fainelli@gmail.com,
jonathanh@nvidia.com, bbiswas@nvidia.com, netdev@vger.kernel.org,
linux-tegra@vger.kernel.org
In-Reply-To: <BN8PR12MB3266A2F1F5F3F18F3A80BFC1D3870@BN8PR12MB3266.namprd12.prod.outlook.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Sep/25/2019, 12:41:04 (UTC+00:00)
> From: David Miller <davem@davemloft.net>
> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
>
> > From: Jose Abreu <Jose.Abreu@synopsys.com>
> > Date: Wed, 25 Sep 2019 10:44:53 +0000
> >
> > > From: David Miller <davem@davemloft.net>
> > > Date: Sep/24/2019, 20:45:08 (UTC+00:00)
> > >
> > >> From: Thierry Reding <thierry.reding@gmail.com>
> > >> Date: Fri, 20 Sep 2019 19:00:34 +0200
> > >>
> > >> Also, you're now writing to the high 32-bits unconditionally, even when
> > >> it will always be zero because of 32-bit addressing. That looks like
> > >> a step backwards to me.
> > >
> > > Don't agree. As per previous discussions and as per my IP knowledge, if
> > > EAME is not enabled / not supported the register can still be written.
> > > This is not fast path and will not impact any remaining operation. Can
> > > you please explain what exactly is the concern about this ?
> > >
> > > Anyway, this is an important feature for performance so I hope Thierry
> > > re-submits this once -next opens and addressing the review comments.
> >
> > Perhaps I misunderstand the context, isn't this code writing the
> > descriptors for every packet?
>
> No, its just setting up the base address for the descriptors which is
> done in open(). The one that's in the fast path is the tail address,
> which is always the lower 32 bits.
Oops, sorry. Indeed it's done in refill operation in function
dwmac4_set_addr() for rx/tx which is fast path so you do have a point
that I was not seeing. Thanks for bringing this up!
Now, the point would be:
a) Is it faster to have an condition check in dwmac4_set_addr(), or
b) Always write to descs the upper 32 bits. Which always exists in the
IP and is a standard write to memory.
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade
From: David Miller @ 2019-09-25 11:48 UTC (permalink / raw)
To: linux; +Cc: andrew, f.fainelli, hkallweit1, tinywrkb, netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Sun, 22 Sep 2019 11:59:32 +0100
> tinywrkb, please can you test this series to ensure that it fixes
> your problem - the previous version has turned out to be a non-starter
> as it introduces more problems, thanks!
>
> The following series attempts to address an issue spotted by tinywrkb
> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> the negotiated link.
...
This doesn't apply cleanly to the current 'net' tree, please respin.
^ permalink raw reply
* Re: [PATCH] atm: he: clean up an indentation issue
From: David Miller @ 2019-09-25 11:55 UTC (permalink / raw)
To: colin.king
Cc: 3chas3, linux-atm-general, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190922114216.10158-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Sun, 22 Sep 2019 13:42:16 +0200
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a statement that is indented one level too many, remove
> the extraneous tab.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: core: dev: replace state xoff flag comparison by netif_xmit_stopped method
From: David Miller @ 2019-09-25 11:56 UTC (permalink / raw)
To: jcfaracco; +Cc: netdev
In-Reply-To: <20190923015729.10273-1-jcfaracco@gmail.com>
From: jcfaracco@gmail.com
Date: Sun, 22 Sep 2019 22:57:29 -0300
> From: Julio Faracco <jcfaracco@gmail.com>
>
> Function netif_schedule_queue() has a hardcoded comparison between queue
> state and any xoff flag. This comparison does the same thing as method
> netif_xmit_stopped(). In terms of code clarity, it is better. See other
> methods like: generic_xdp_tx() and dev_direct_xmit().
>
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
net-next is closed, please resubmit when the net-next tree opens back up.
Thank you.
^ permalink raw reply
* Re: [PATCH net] gianfar: Make reset_gfar static
From: David Miller @ 2019-09-25 11:57 UTC (permalink / raw)
To: yuehaibing; +Cc: claudiu.manoil, jakub.kicinski, netdev, linux-kernel
In-Reply-To: <20190923061603.37064-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Mon, 23 Sep 2019 14:16:03 +0800
> Fix sparse warning:
>
> drivers/net/ethernet/freescale/gianfar.c:2070:6:
> warning: symbol 'reset_gfar' was not declared. Should it be static?
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 5/6] vringh: fix copy direction of vringh_iov_push_kern()
From: Jason Wang @ 2019-09-25 11:56 UTC (permalink / raw)
To: Alex Williamson, Michael S. Tsirkin
Cc: kvm, linux-s390, linux-kernel, dri-devel, intel-gfx,
intel-gvt-dev, kwankhede, tiwei.bie, virtualization, netdev,
cohuck, maxime.coquelin, cunming.liang, zhihong.wang, rob.miller,
xiao.w.wang, haotian.wang, zhenyuw, zhi.a.wang, jani.nikula,
joonas.lahtinen, rodrigo.vivi, airlied, daniel, farman, pasic,
sebott, oberpar, heiko.carstens, gor, borntraeger, akrowiak,
freude, lingshan.zhu, idos, eperezma, lulu, parav
In-Reply-To: <20190924080413.0cc875c5@x1.home>
On 2019/9/24 下午10:04, Alex Williamson wrote:
> On Mon, 23 Sep 2019 12:00:41 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Mon, Sep 23, 2019 at 09:45:59AM -0600, Alex Williamson wrote:
>>> On Mon, 23 Sep 2019 21:03:30 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>
>>>> We want to copy from iov to buf, so the direction was wrong.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>> drivers/vhost/vringh.c | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>> Why is this included in the series? Seems like an unrelated fix being
>>> held up within a proposal for a new feature. Thanks,
>>>
>>> Alex
>> It's better to have it as patch 1/6, but it's a dependency of the
>> example driver in the series. I can reorder when I apply.
> It's a fix, please submit it separately through virtio/vhost channels,
> then it will already be in the base kernel we use for the rest of the
> series. The remainder of the series certainly suggests a workflow
> through the vfio tree rather than virtio/vhost. Thanks,
>
> Alex
Ok.
Thanks
^ permalink raw reply
* Re: [PATCH net] net: aquantia: Fix aq_vec_isr_legacy() return value
From: Igor Russkikh @ 2019-09-25 11:57 UTC (permalink / raw)
To: Dan Carpenter, David VomLehn
Cc: David S. Miller, netdev@vger.kernel.org,
kernel-janitors@vger.kernel.org
In-Reply-To: <20190925105430.GA3264@mwanda>
On 25.09.2019 13:54, Dan Carpenter wrote:
> The irqreturn_t type is an enum or an unsigned int in GCC. That
> creates to problems because it can't detect if the
> self->aq_hw_ops->hw_irq_read() call fails and at the end the function
> always returns IRQ_HANDLED.
Thanks for noticing this, Dan!
> drivers/net/ethernet/aquantia/atlantic/aq_vec.c:316 aq_vec_isr_legacy() warn: unsigned 'err' is never less than zero.
> drivers/net/ethernet/aquantia/atlantic/aq_vec.c:329 aq_vec_isr_legacy() warn: always true condition '(err >= 0) => (0-u32max >= 0)'
>
> Fixes: 970a2e9864b0 ("net: ethernet: aquantia: Vector operations")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Igor Russkikh <igor.russkikh@aquantia.com>
Regards,
Igor
^ permalink raw reply
* Re: [PATCH net v2] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: David Miller @ 2019-09-25 11:58 UTC (permalink / raw)
To: Jose.Abreu
Cc: netdev, Joao.Pinto, peppe.cavallaro, alexandre.torgue,
mcoquelin.stm32, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <efc49b6ea500115ff1442ddebe9dc7d956eb19e9.1569224900.git.Jose.Abreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Mon, 23 Sep 2019 09:49:08 +0200
> The Flow Control selftest is also available with ASYM Pause. Lets add
> this check to the test and fix eventual false positive failures.
>
> Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
Applied.
^ permalink raw reply
* Re: [RFC PATCH] net: macb: Remove dead code
From: David Miller @ 2019-09-25 12:00 UTC (permalink / raw)
To: shubhrajyoti.datta; +Cc: netdev, nicolas.ferre, shubhrajyoti.datta
In-Reply-To: <1569226782-19635-1-git-send-email-shubhrajyoti.datta@xilinx.com>
From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Date: Mon, 23 Sep 2019 13:49:42 +0530
> macb_64b_desc is always called when HW_DMA_CAP_64B is defined.
> So the return NULL can never be reached. Remove the dead code.
>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Applied.
^ permalink raw reply
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: David Miller @ 2019-09-25 12:01 UTC (permalink / raw)
To: Jose.Abreu
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
In-Reply-To: <BN8PR12MB3266A2F1F5F3F18F3A80BFC1D3870@BN8PR12MB3266.namprd12.prod.outlook.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Wed, 25 Sep 2019 11:41:04 +0000
> From: David Miller <davem@davemloft.net>
> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
>
>> From: Jose Abreu <Jose.Abreu@synopsys.com>
>> Date: Wed, 25 Sep 2019 10:44:53 +0000
>>
>> > From: David Miller <davem@davemloft.net>
>> > Date: Sep/24/2019, 20:45:08 (UTC+00:00)
>> >
>> >> From: Thierry Reding <thierry.reding@gmail.com>
>> >> Date: Fri, 20 Sep 2019 19:00:34 +0200
>> >>
>> >> Also, you're now writing to the high 32-bits unconditionally, even when
>> >> it will always be zero because of 32-bit addressing. That looks like
>> >> a step backwards to me.
>> >
>> > Don't agree. As per previous discussions and as per my IP knowledge, if
>> > EAME is not enabled / not supported the register can still be written.
>> > This is not fast path and will not impact any remaining operation. Can
>> > you please explain what exactly is the concern about this ?
>> >
>> > Anyway, this is an important feature for performance so I hope Thierry
>> > re-submits this once -next opens and addressing the review comments.
>>
>> Perhaps I misunderstand the context, isn't this code writing the
>> descriptors for every packet?
>
> No, its just setting up the base address for the descriptors which is
> done in open(). The one that's in the fast path is the tail address,
> which is always the lower 32 bits.
Aha, ok, yes then initializing both parts unconditionally is fine.
^ permalink raw reply
* RE: [PATCH net] net: axienet: fix a signedness bug in probe
From: Radhey Shyam Pandey @ 2019-09-25 12:01 UTC (permalink / raw)
To: Dan Carpenter, Alvaro G. M
Cc: David S. Miller, Michal Simek, Russell King,
netdev@vger.kernel.org, kernel-janitors@vger.kernel.org
In-Reply-To: <20190925105911.GI3264@mwanda>
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, September 25, 2019 4:29 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>; Alvaro G. M
> <alvaro.gamez@hazent.com>
> Cc: David S. Miller <davem@davemloft.net>; Michal Simek
> <michals@xilinx.com>; Russell King <linux@armlinux.org.uk>;
> netdev@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: [PATCH net] net: axienet: fix a signedness bug in probe
>
> The "lp->phy_mode" is an enum but in this context GCC treats it as an
> unsigned int so the error handling is never triggered.
>
> Fixes: ee06b1728b95 ("net: axienet: add support for standard phy-mode
> binding")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 4fc627fb4d11..676006f32f91 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1762,7 +1762,7 @@ static int axienet_probe(struct platform_device
> *pdev)
> }
> } else {
> lp->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> - if (lp->phy_mode < 0) {
> + if ((int)lp->phy_mode < 0) {
> ret = -EINVAL;
> goto free_netdev;
> }
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH V2 2/8] mdev: class id support
From: Jason Wang @ 2019-09-25 12:01 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-s390, linux-kernel, dri-devel, intel-gfx,
intel-gvt-dev, kwankhede, mst, tiwei.bie, virtualization, netdev,
cohuck, maxime.coquelin, cunming.liang, zhihong.wang, rob.miller,
xiao.w.wang, haotian.wang, zhenyuw, zhi.a.wang, jani.nikula,
joonas.lahtinen, rodrigo.vivi, airlied, daniel, farman, pasic,
sebott, oberpar, heiko.carstens, gor, borntraeger, akrowiak,
freude, lingshan.zhu, idos, eperezma, lulu, parav,
christophe.de.dinechin, kevin.tian
In-Reply-To: <20190924170627.083f9f1b@x1.home>
On 2019/9/25 上午7:06, Alex Williamson wrote:
> On Tue, 24 Sep 2019 21:53:26 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Mdev bus only supports vfio driver right now, so it doesn't implement
>> match method. But in the future, we may add drivers other than vfio,
>> the first driver could be virtio-mdev. This means we need to add
>> device class id support in bus match method to pair the mdev device
>> and mdev driver correctly.
>>
>> So this patch adds id_table to mdev_driver and class_id for mdev
>> parent with the match method for mdev bus.
> Description needs to be revised from v1, class_id is no longer on the
> parent.
Yes, will fix.
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Documentation/driver-api/vfio-mediated-device.rst | 3 +++
>> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 +
>> drivers/s390/cio/vfio_ccw_ops.c | 1 +
>> drivers/s390/crypto/vfio_ap_ops.c | 1 +
>> drivers/vfio/mdev/mdev_core.c | 7 +++++++
>> drivers/vfio/mdev/mdev_driver.c | 14 ++++++++++++++
>> drivers/vfio/mdev/mdev_private.h | 1 +
>> drivers/vfio/mdev/vfio_mdev.c | 6 ++++++
>> include/linux/mdev.h | 8 ++++++++
>> include/linux/mod_devicetable.h | 8 ++++++++
>> samples/vfio-mdev/mbochs.c | 1 +
>> samples/vfio-mdev/mdpy.c | 1 +
>> samples/vfio-mdev/mtty.c | 1 +
>> 13 files changed, 53 insertions(+)
>>
>> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
>> index 25eb7d5b834b..a5bdc60d62a1 100644
>> --- a/Documentation/driver-api/vfio-mediated-device.rst
>> +++ b/Documentation/driver-api/vfio-mediated-device.rst
>> @@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
>> * @probe: called when new device created
>> * @remove: called when device removed
>> * @driver: device driver structure
>> + * @id_table: the ids serviced by this driver
>> */
>> struct mdev_driver {
>> const char *name;
>> int (*probe) (struct device *dev);
>> void (*remove) (struct device *dev);
>> struct device_driver driver;
>> + const struct mdev_class_id *id_table;
>> };
>>
>> A mediated bus driver for mdev should use this structure in the function calls
>> @@ -165,6 +167,7 @@ register itself with the mdev core driver::
>> extern int mdev_register_device(struct device *dev,
>> const struct mdev_parent_ops *ops);
>>
>> +
>> However, the mdev_parent_ops structure is not required in the function call
>> that a driver should use to unregister itself with the mdev core driver::
> Unintended extra line? Doesn't seem to match surrounding formatting.
>
> Calling mdev_set_class_id() as part of create seems relatively
> fundamental to the vendor driver with this change, it should be added
> to the documentation.
Right.
>
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 23aa3e50cbf8..f793252a3d2a 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>> dev_name(mdev_dev(mdev)));
>> ret = 0;
>>
>> + mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> out:
>> return ret;
>> }
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index f0d71ab77c50..d258ef1fedb9 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -129,6 +129,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>> private->sch->schid.ssid,
>> private->sch->schid.sch_no);
>>
>> + mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> return 0;
>> }
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 5c0f53c6dde7..2cfd96112aa0 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -343,6 +343,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>> list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>> mutex_unlock(&matrix_dev->lock);
>>
>> + mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> return 0;
>> }
>>
>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>> index b558d4cfd082..8764cf4a276d 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -45,6 +45,12 @@ void mdev_set_drvdata(struct mdev_device *mdev, void *data)
>> }
>> EXPORT_SYMBOL(mdev_set_drvdata);
>>
>> +void mdev_set_class_id(struct mdev_device *mdev, u16 id)
>> +{
>> + mdev->class_id = id;
>> +}
>> +EXPORT_SYMBOL(mdev_set_class_id);
>> +
>> struct device *mdev_dev(struct mdev_device *mdev)
>> {
>> return &mdev->dev;
>> @@ -135,6 +141,7 @@ static int mdev_device_remove_cb(struct device *dev, void *data)
>> * mdev_register_device : Register a device
>> * @dev: device structure representing parent device.
>> * @ops: Parent device operation structure to be registered.
>> + * @id: device id.
>> *
>> * Add device to list of registered parent devices.
>> * Returns a negative value on error, otherwise 0.
>> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
>> index 0d3223aee20b..b7c40ce86ee3 100644
>> --- a/drivers/vfio/mdev/mdev_driver.c
>> +++ b/drivers/vfio/mdev/mdev_driver.c
>> @@ -69,8 +69,22 @@ static int mdev_remove(struct device *dev)
>> return 0;
>> }
>>
>> +static int mdev_match(struct device *dev, struct device_driver *drv)
>> +{
>> + unsigned int i;
>> + struct mdev_device *mdev = to_mdev_device(dev);
>> + struct mdev_driver *mdrv = to_mdev_driver(drv);
>> + const struct mdev_class_id *ids = mdrv->id_table;
>> +
>> + for (i = 0; ids[i].id; i++)
>> + if (ids[i].id == mdev->class_id)
> With the proposed API here, mdev->class_id can be NULL, do we allow
> devices to exist in that state or should mdev_device_create() generate
> an error if the .create() callback doesn't register a type? Or a
> warn_once and assume MDEV_ID_VFIO? The latter seems pretty sketchy
> when we get to patch 5/8. Thanks,
It looks to me the error from mdev_device_create() is better.
Thanks
>
> Alex
>
>> + return 1;
>> + return 0;
>> +}
>> +
>> struct bus_type mdev_bus_type = {
>> .name = "mdev",
>> + .match = mdev_match,
>> .probe = mdev_probe,
>> .remove = mdev_remove,
>> };
>> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
>> index 7d922950caaf..c65f436c1869 100644
>> --- a/drivers/vfio/mdev/mdev_private.h
>> +++ b/drivers/vfio/mdev/mdev_private.h
>> @@ -33,6 +33,7 @@ struct mdev_device {
>> struct kobject *type_kobj;
>> struct device *iommu_device;
>> bool active;
>> + u16 class_id;
>> };
>>
>> #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev)
>> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
>> index 30964a4e0a28..fd2a4d9a3f26 100644
>> --- a/drivers/vfio/mdev/vfio_mdev.c
>> +++ b/drivers/vfio/mdev/vfio_mdev.c
>> @@ -120,10 +120,16 @@ static void vfio_mdev_remove(struct device *dev)
>> vfio_del_group_dev(dev);
>> }
>>
>> +static struct mdev_class_id id_table[] = {
>> + { MDEV_ID_VFIO },
>> + { 0 },
>> +};
>> +
>> static struct mdev_driver vfio_mdev_driver = {
>> .name = "vfio_mdev",
>> .probe = vfio_mdev_probe,
>> .remove = vfio_mdev_remove,
>> + .id_table = id_table,
>> };
>>
>> static int __init vfio_mdev_init(void)
>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
>> index 0ce30ca78db0..3974650c074f 100644
>> --- a/include/linux/mdev.h
>> +++ b/include/linux/mdev.h
>> @@ -118,6 +118,7 @@ struct mdev_type_attribute mdev_type_attr_##_name = \
>> * @probe: called when new device created
>> * @remove: called when device removed
>> * @driver: device driver structure
>> + * @id_table: the ids serviced by this driver.
>> *
>> **/
>> struct mdev_driver {
>> @@ -125,12 +126,14 @@ struct mdev_driver {
>> int (*probe)(struct device *dev);
>> void (*remove)(struct device *dev);
>> struct device_driver driver;
>> + const struct mdev_class_id *id_table;
>> };
>>
>> #define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver)
>>
>> void *mdev_get_drvdata(struct mdev_device *mdev);
>> void mdev_set_drvdata(struct mdev_device *mdev, void *data);
>> +void mdev_set_class_id(struct mdev_device *mdev, u16 id);
>> const guid_t *mdev_uuid(struct mdev_device *mdev);
>>
>> extern struct bus_type mdev_bus_type;
>> @@ -145,4 +148,9 @@ struct device *mdev_parent_dev(struct mdev_device *mdev);
>> struct device *mdev_dev(struct mdev_device *mdev);
>> struct mdev_device *mdev_from_dev(struct device *dev);
>>
>> +enum {
>> + MDEV_ID_VFIO = 1,
>> + /* New entries must be added here */
>> +};
>> +
>> #endif /* MDEV_H */
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index 5714fd35a83c..f32c6e44fb1a 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -821,4 +821,12 @@ struct wmi_device_id {
>> const void *context;
>> };
>>
>> +/**
>> + * struct mdev_class_id - MDEV device class identifier
>> + * @id: Used to identify a specific class of device, e.g vfio-mdev device.
>> + */
>> +struct mdev_class_id {
>> + __u16 id;
>> +};
>> +
>> #endif /* LINUX_MOD_DEVICETABLE_H */
>> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
>> index ac5c8c17b1ff..8a8583c892b2 100644
>> --- a/samples/vfio-mdev/mbochs.c
>> +++ b/samples/vfio-mdev/mbochs.c
>> @@ -561,6 +561,7 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)
>> mbochs_reset(mdev);
>>
>> mbochs_used_mbytes += type->mbytes;
>> + mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> return 0;
>>
>> err_mem:
>> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
>> index cc86bf6566e4..88d7e76f3836 100644
>> --- a/samples/vfio-mdev/mdpy.c
>> +++ b/samples/vfio-mdev/mdpy.c
>> @@ -269,6 +269,7 @@ static int mdpy_create(struct kobject *kobj, struct mdev_device *mdev)
>> mdpy_reset(mdev);
>>
>> mdpy_count++;
>> + mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> return 0;
>> }
>>
>> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
>> index 92e770a06ea2..4e0735143b69 100644
>> --- a/samples/vfio-mdev/mtty.c
>> +++ b/samples/vfio-mdev/mtty.c
>> @@ -770,6 +770,7 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev)
>> list_add(&mdev_state->next, &mdev_devices_list);
>> mutex_unlock(&mdev_list_lock);
>>
>> + mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> return 0;
>> }
>>
^ permalink raw reply
* Re: [PATCH V2 5/8] mdev: introduce device specific ops
From: Jason Wang @ 2019-09-25 12:04 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-s390, linux-kernel, dri-devel, intel-gfx,
intel-gvt-dev, kwankhede, mst, tiwei.bie, virtualization, netdev,
cohuck, maxime.coquelin, cunming.liang, zhihong.wang, rob.miller,
xiao.w.wang, haotian.wang, zhenyuw, zhi.a.wang, jani.nikula,
joonas.lahtinen, rodrigo.vivi, airlied, daniel, farman, pasic,
sebott, oberpar, heiko.carstens, gor, borntraeger, akrowiak,
freude, lingshan.zhu, idos, eperezma, lulu, parav,
christophe.de.dinechin, kevin.tian
In-Reply-To: <20190924170638.064d85f7@x1.home>
On 2019/9/25 上午7:06, Alex Williamson wrote:
> On Tue, 24 Sep 2019 21:53:29 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Currently, except for the create and remove, the rest of
>> mdev_parent_ops is designed for vfio-mdev driver only and may not help
>> for kernel mdev driver. With the help of class id, this patch
>> introduces device specific callbacks inside mdev_device
>> structure. This allows different set of callback to be used by
>> vfio-mdev and virtio-mdev.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> .../driver-api/vfio-mediated-device.rst | 4 +-
>> MAINTAINERS | 1 +
>> drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +++---
>> drivers/s390/cio/vfio_ccw_ops.c | 17 ++++--
>> drivers/s390/crypto/vfio_ap_ops.c | 13 +++--
>> drivers/vfio/mdev/mdev_core.c | 12 +++++
>> drivers/vfio/mdev/mdev_private.h | 1 +
>> drivers/vfio/mdev/vfio_mdev.c | 37 ++++++-------
>> include/linux/mdev.h | 42 ++++-----------
>> include/linux/vfio_mdev.h | 52 +++++++++++++++++++
>> samples/vfio-mdev/mbochs.c | 19 ++++---
>> samples/vfio-mdev/mdpy.c | 19 ++++---
>> samples/vfio-mdev/mtty.c | 17 ++++--
>> 13 files changed, 168 insertions(+), 83 deletions(-)
>> create mode 100644 include/linux/vfio_mdev.h
>>
>> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
>> index a5bdc60d62a1..d50425b368bb 100644
>> --- a/Documentation/driver-api/vfio-mediated-device.rst
>> +++ b/Documentation/driver-api/vfio-mediated-device.rst
>> @@ -152,7 +152,9 @@ callbacks per mdev parent device, per mdev type, or any other categorization.
>> Vendor drivers are expected to be fully asynchronous in this respect or
>> provide their own internal resource protection.)
>>
>> -The callbacks in the mdev_parent_ops structure are as follows:
>> +The device specific callbacks are referred through device_ops pointer
>> +in mdev_parent_ops. For vfio-mdev device, its callbacks in device_ops
>> +are as follows:
> This is not accurate. device_ops is now on the mdev_device and is an
> mdev bus driver specific structure of callbacks that must be registered
> for each mdev device by the parent driver during the create callback.
> There's a one to one mapping of class_id to mdev_device_ops callbacks.
Yes.
>
> That also suggests to me that we could be more clever in registering
> both of these with mdev-core. Can we embed the class_id in the ops
> structure in a common way so that the core can extract it and the bus
> drivers can access their specific callbacks?
That seems much cleaner, let me try to do that in V3.
>
>> * open: open callback of mediated device
>> * close: close callback of mediated device
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b2326dece28e..89832b316500 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -17075,6 +17075,7 @@ S: Maintained
>> F: Documentation/driver-api/vfio-mediated-device.rst
>> F: drivers/vfio/mdev/
>> F: include/linux/mdev.h
>> +F: include/linux/vfio_mdev.h
>> F: samples/vfio-mdev/
>>
>> VFIO PLATFORM DRIVER
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index f793252a3d2a..b274f5ee481f 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -42,6 +42,7 @@
>> #include <linux/kvm_host.h>
>> #include <linux/vfio.h>
>> #include <linux/mdev.h>
>> +#include <linux/vfio_mdev.h>
>> #include <linux/debugfs.h>
>>
>> #include <linux/nospec.h>
>> @@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu)
>> vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
>> }
>>
>> +static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;
>> +
>> static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>> {
>> struct intel_vgpu *vgpu = NULL;
>> @@ -679,6 +682,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>> ret = 0;
>>
>> mdev_set_class_id(mdev, MDEV_ID_VFIO);
>> + mdev_set_dev_ops(mdev, &intel_vfio_vgpu_dev_ops);
> This seems rather unrefined. We're registering interdependent data in
> separate calls. All drivers need to make both of these calls. I'm not
> sure if this is a good idea, but what if we had:
>
> static const struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = {
> .id = MDEV_ID_VFIO,
> .open = intel_vgpu_open,
> .release = intel_vgpu_release,
> ...
>
> And the set function passed &intel_vfio_vgpu_dev_ops.id and the mdev
> bus drivers used container_of to get to their callbacks?
Yes, with the check of mdev_device_create() if nothing is set by the device.
>
>> out:
>> return ret;
>> }
>> @@ -1601,20 +1605,21 @@ static const struct attribute_group *intel_vgpu_groups[] = {
>> NULL,
>> };
>>
>> -static struct mdev_parent_ops intel_vgpu_ops = {
>> - .mdev_attr_groups = intel_vgpu_groups,
>> - .create = intel_vgpu_create,
>> - .remove = intel_vgpu_remove,
>> -
>> +static struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = {
>> .open = intel_vgpu_open,
>> .release = intel_vgpu_release,
>> -
>> .read = intel_vgpu_read,
>> .write = intel_vgpu_write,
>> .mmap = intel_vgpu_mmap,
>> .ioctl = intel_vgpu_ioctl,
>> };
>>
>> +static struct mdev_parent_ops intel_vgpu_ops = {
> These could maybe be made const at the same time. Thanks,
>
> Alex
Ok, let me fix.
Thanks
^ permalink raw reply
* Re: [PATCH V2 6/8] mdev: introduce virtio device and its device ops
From: Jason Wang @ 2019-09-25 12:06 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-s390, linux-kernel, dri-devel, intel-gfx,
intel-gvt-dev, kwankhede, mst, tiwei.bie, virtualization, netdev,
cohuck, maxime.coquelin, cunming.liang, zhihong.wang, rob.miller,
xiao.w.wang, haotian.wang, zhenyuw, zhi.a.wang, jani.nikula,
joonas.lahtinen, rodrigo.vivi, airlied, daniel, farman, pasic,
sebott, oberpar, heiko.carstens, gor, borntraeger, akrowiak,
freude, lingshan.zhu, idos, eperezma, lulu, parav,
christophe.de.dinechin, kevin.tian
In-Reply-To: <20190924170640.1da03bae@x1.home>
On 2019/9/25 上午7:06, Alex Williamson wrote:
> On Tue, 24 Sep 2019 21:53:30 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> This patch implements basic support for mdev driver that supports
>> virtio transport for kernel virtio driver.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> include/linux/mdev.h | 2 +
>> include/linux/virtio_mdev.h | 145 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 147 insertions(+)
>> create mode 100644 include/linux/virtio_mdev.h
>>
>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
>> index 3414307311f1..73ac27b3b868 100644
>> --- a/include/linux/mdev.h
>> +++ b/include/linux/mdev.h
>> @@ -126,6 +126,8 @@ struct mdev_device *mdev_from_dev(struct device *dev);
>>
>> enum {
>> MDEV_ID_VFIO = 1,
>> + MDEV_ID_VIRTIO = 2,
>> + MDEV_ID_VHOST = 3,
> MDEV_ID_VHOST isn't used yet here.
Yes, just want to reserve one but it look too early to do that.
> Also, given the strong
> interdependence between the class_id and the ops structure, we might
> wand to define them in the same place. Thanks,
>
> Alex
Yes, as you suggest in patch 5/8.
Thanks
>
>> /* New entries must be added here */
>> };
>>
>> diff --git a/include/linux/virtio_mdev.h b/include/linux/virtio_mdev.h
>> new file mode 100644
>> index 000000000000..d1a40a739266
>> --- /dev/null
>> +++ b/include/linux/virtio_mdev.h
>> @@ -0,0 +1,145 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Virtio mediated device driver
>> + *
>> + * Copyright 2019, Red Hat Corp.
>> + * Author: Jason Wang <jasowang@redhat.com>
>> + */
>> +#ifndef _LINUX_VIRTIO_MDEV_H
>> +#define _LINUX_VIRTIO_MDEV_H
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/mdev.h>
>> +#include <uapi/linux/vhost.h>
>> +
>> +#define VIRTIO_MDEV_DEVICE_API_STRING "virtio-mdev"
>> +#define VIRTIO_MDEV_VERSION 0x1
>> +
>> +struct virtio_mdev_callback {
>> + irqreturn_t (*callback)(void *data);
>> + void *private;
>> +};
>> +
>> +/**
>> + * struct vfio_mdev_device_ops - Structure to be registered for each
>> + * mdev device to register the device to virtio-mdev module.
>> + *
>> + * @set_vq_address: Set the address of virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * @desc_area: address of desc area
>> + * @driver_area: address of driver area
>> + * @device_area: address of device area
>> + * Returns integer: success (0) or error (< 0)
>> + * @set_vq_num: Set the size of virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * @num: the size of virtqueue
>> + * @kick_vq: Kick the virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * @set_vq_cb: Set the interrut calback function for
>> + * a virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * @cb: virtio-mdev interrupt callback structure
>> + * @set_vq_ready: Set ready status for a virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * @ready: ready (true) not ready(false)
>> + * @get_vq_ready: Get ready status for a virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * Returns boolean: ready (true) or not (false)
>> + * @set_vq_state: Set the state for a virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * @state: virtqueue state (last_avail_idx)
>> + * Returns integer: success (0) or error (< 0)
>> + * @get_vq_state: Get the state for a virtqueue
>> + * @mdev: mediated device
>> + * @idx: virtqueue index
>> + * Returns virtqueue state (last_avail_idx)
>> + * @get_vq_align: Get the virtqueue align requirement
>> + * for the device
>> + * @mdev: mediated device
>> + * Returns virtqueue algin requirement
>> + * @get_features: Get virtio features supported by the device
>> + * @mdev: mediated device
>> + * Returns the features support by the
>> + * device
>> + * @get_features: Set virtio features supported by the driver
>> + * @mdev: mediated device
>> + * @features: feature support by the driver
>> + * Returns integer: success (0) or error (< 0)
>> + * @set_config_cb: Set the config interrupt callback
>> + * @mdev: mediated device
>> + * @cb: virtio-mdev interrupt callback structure
>> + * @get_device_id: Get virtio device id
>> + * @mdev: mediated device
>> + * Returns u32: virtio device id
>> + * @get_vendor_id: Get virtio vendor id
>> + * @mdev: mediated device
>> + * Returns u32: virtio vendor id
>> + * @get_status: Get the device status
>> + * @mdev: mediated device
>> + * Returns u8: virtio device status
>> + * @set_status: Set the device status
>> + * @mdev: mediated device
>> + * @status: virtio device status
>> + * @get_config: Read from device specific confiugration space
>> + * @mdev: mediated device
>> + * @offset: offset from the beginning of
>> + * configuration space
>> + * @buf: buffer used to read to
>> + * @len: the length to read from
>> + * configration space
>> + * @set_config: Write to device specific confiugration space
>> + * @mdev: mediated device
>> + * @offset: offset from the beginning of
>> + * configuration space
>> + * @buf: buffer used to write from
>> + * @len: the length to write to
>> + * configration space
>> + * @get_version: Get the version of virtio mdev device
>> + * @mdev: mediated device
>> + * Returns integer: version of the device
>> + * @get_generation: Get device generaton
>> + * @mdev: mediated device
>> + * Returns u32: device generation
>> + */
>> +struct virtio_mdev_device_ops {
>> + /* Virtqueue ops */
>> + int (*set_vq_address)(struct mdev_device *mdev,
>> + u16 idx, u64 desc_area, u64 driver_area,
>> + u64 device_area);
>> + void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
>> + void (*kick_vq)(struct mdev_device *mdev, u16 idx);
>> + void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,
>> + struct virtio_mdev_callback *cb);
>> + void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool ready);
>> + bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
>> + int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
>> + u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
>> +
>> + /* Device ops */
>> + u16 (*get_vq_align)(struct mdev_device *mdev);
>> + u64 (*get_features)(struct mdev_device *mdev);
>> + int (*set_features)(struct mdev_device *mdev, u64 features);
>> + void (*set_config_cb)(struct mdev_device *mdev,
>> + struct virtio_mdev_callback *cb);
>> + u16 (*get_queue_max)(struct mdev_device *mdev);
>> + u32 (*get_device_id)(struct mdev_device *mdev);
>> + u32 (*get_vendor_id)(struct mdev_device *mdev);
>> + u8 (*get_status)(struct mdev_device *mdev);
>> + void (*set_status)(struct mdev_device *mdev, u8 status);
>> + void (*get_config)(struct mdev_device *mdev, unsigned int offset,
>> + void *buf, unsigned int len);
>> + void (*set_config)(struct mdev_device *mdev, unsigned int offset,
>> + const void *buf, unsigned int len);
>> + int (*get_version)(struct mdev_device *mdev);
>> + u32 (*get_generation)(struct mdev_device *mdev);
>> +};
>> +
>> +#endif
>> +
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox