Hello Gowri, On Tue, Dec 24, 2024 at 8:39 AM Gowrishankar Muthukrishnan <gmuthukri...@marvell.com> wrote: > > Common virtio log include file.
That's really a short commitlog.. What are you trying to achieve? The net/virtio and crypto/virtio drivers had dedicated logtypes so far, which seems preferable. I don't see a case when using a single logtype for both net and crypto would help. Some comments below. And please run checkpatch. > > Signed-off-by: Gowrishankar Muthukrishnan <gmuthukri...@marvell.com> > --- > drivers/{net => common}/virtio/virtio_logs.h | 16 ++-------- > drivers/crypto/virtio/meson.build | 1 + > .../{virtio_logs.h => virtio_crypto_logs.h} | 30 ++++++++----------- > drivers/crypto/virtio/virtio_cryptodev.c | 4 +-- > drivers/crypto/virtio/virtqueue.h | 2 +- > drivers/net/virtio/meson.build | 3 +- > drivers/net/virtio/virtio.c | 3 +- > drivers/net/virtio/virtio_ethdev.c | 3 +- > drivers/net/virtio/virtio_net_logs.h | 30 +++++++++++++++++++ > drivers/net/virtio/virtio_pci.c | 3 +- > drivers/net/virtio/virtio_pci_ethdev.c | 3 +- > drivers/net/virtio/virtio_rxtx.c | 3 +- > drivers/net/virtio/virtio_rxtx_packed.c | 3 +- > drivers/net/virtio/virtio_rxtx_packed.h | 3 +- > drivers/net/virtio/virtio_rxtx_packed_avx.h | 3 +- > drivers/net/virtio/virtio_rxtx_simple.h | 3 +- > .../net/virtio/virtio_user/vhost_kernel_tap.c | 3 +- > drivers/net/virtio/virtio_user/vhost_vdpa.c | 3 +- > drivers/net/virtio/virtio_user_ethdev.c | 3 +- > drivers/net/virtio/virtqueue.c | 3 +- > drivers/net/virtio/virtqueue.h | 3 +- > 21 files changed, 77 insertions(+), 51 deletions(-) > rename drivers/{net => common}/virtio/virtio_logs.h (61%) > rename drivers/crypto/virtio/{virtio_logs.h => virtio_crypto_logs.h} (74%) > create mode 100644 drivers/net/virtio/virtio_net_logs.h > > diff --git a/drivers/net/virtio/virtio_logs.h > b/drivers/common/virtio/virtio_logs.h > similarity index 61% > rename from drivers/net/virtio/virtio_logs.h > rename to drivers/common/virtio/virtio_logs.h > index dea1a7ac11..bc115e7a36 100644 > --- a/drivers/net/virtio/virtio_logs.h > +++ b/drivers/common/virtio/virtio_logs.h > @@ -5,6 +5,8 @@ > #ifndef _VIRTIO_LOGS_H_ > #define _VIRTIO_LOGS_H_ > > +#include <inttypes.h> > + ? Seems unrelated. > #include <rte_log.h> > > extern int virtio_logtype_init; > @@ -14,20 +16,6 @@ extern int virtio_logtype_init; > > #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>") > > -#ifdef RTE_LIBRTE_VIRTIO_DEBUG_RX > -#define PMD_RX_LOG(level, ...) \ > - RTE_LOG_LINE_PREFIX(level, VIRTIO_DRIVER, "%s() rx: ", __func__, > __VA_ARGS__) > -#else > -#define PMD_RX_LOG(...) do { } while(0) > -#endif > - > -#ifdef RTE_LIBRTE_VIRTIO_DEBUG_TX > -#define PMD_TX_LOG(level, ...) \ > - RTE_LOG_LINE_PREFIX(level, VIRTIO_DRIVER, "%s() tx: ", __func__, > __VA_ARGS__) > -#else > -#define PMD_TX_LOG(...) do { } while(0) > -#endif > - > extern int virtio_logtype_driver; > #define RTE_LOGTYPE_VIRTIO_DRIVER virtio_logtype_driver > #define PMD_DRV_LOG(level, ...) \ > diff --git a/drivers/crypto/virtio/meson.build > b/drivers/crypto/virtio/meson.build > index d2c3b3ad07..6c082a3112 100644 > --- a/drivers/crypto/virtio/meson.build > +++ b/drivers/crypto/virtio/meson.build > @@ -8,6 +8,7 @@ if is_windows > endif > > includes += include_directories('../../../lib/vhost') > +includes += include_directories('../../common/virtio') There are some special cases when a driver can't rely on meson dependencies (like order of subdirs evaluation in drivers/meson.build). For those special cases, include_directories are used. But this driver does not seem concerned. There should be dependencies on vhost and common_virtio instead. > deps += 'bus_pci' > sources = files( > 'virtio_cryptodev.c', > diff --git a/drivers/crypto/virtio/virtio_logs.h > b/drivers/crypto/virtio/virtio_crypto_logs.h > similarity index 74% > rename from drivers/crypto/virtio/virtio_logs.h > rename to drivers/crypto/virtio/virtio_crypto_logs.h > index 988514919f..56caa162d4 100644 > --- a/drivers/crypto/virtio/virtio_logs.h > +++ b/drivers/crypto/virtio/virtio_crypto_logs.h > @@ -2,24 +2,18 @@ > * Copyright(c) 2018 HUAWEI TECHNOLOGIES CO., LTD. > */ > > -#ifndef _VIRTIO_LOGS_H_ > -#define _VIRTIO_LOGS_H_ > +#ifndef _VIRTIO_CRYPTO_LOGS_H_ > +#define _VIRTIO_CRYPTO_LOGS_H_ > > #include <rte_log.h> > > -extern int virtio_crypto_logtype_init; > -#define RTE_LOGTYPE_VIRTIO_CRYPTO_INIT virtio_crypto_logtype_init > +#include "virtio_logs.h" > > -#define PMD_INIT_LOG(level, ...) \ > - RTE_LOG_LINE_PREFIX(level, VIRTIO_CRYPTO_INIT, "%s(): ", __func__, > __VA_ARGS__) > +extern int virtio_logtype_init; > > -#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>") > - > -extern int virtio_crypto_logtype_init; > -#define RTE_LOGTYPE_VIRTIO_CRYPTO_INIT virtio_crypto_logtype_init > - > -#define VIRTIO_CRYPTO_INIT_LOG_IMPL(level, ...) \ > - RTE_LOG_LINE_PREFIX(level, VIRTIO_CRYPTO_INIT, "%s(): ", __func__, > __VA_ARGS__) > +#define VIRTIO_CRYPTO_INIT_LOG_IMPL(level, fmt, args...) \ > + rte_log(RTE_LOG_ ## level, virtio_logtype_init, \ > + "INIT: %s(): " fmt "\n", __func__, ##args) Don't add back macros directly calling rte_log(). Afaiu, this hunk should be only redirecting RTE_LOGTYPE_VIRTIO_CRYPTO_INIT to virtio_logtype_init. > > #define VIRTIO_CRYPTO_INIT_LOG_INFO(fmt, ...) \ > VIRTIO_CRYPTO_INIT_LOG_IMPL(INFO, fmt, ## __VA_ARGS__) > @@ -75,11 +69,11 @@ extern int virtio_crypto_logtype_tx; > #define VIRTIO_CRYPTO_TX_LOG_ERR(fmt, ...) \ > VIRTIO_CRYPTO_TX_LOG_IMPL(ERR, fmt, ## __VA_ARGS__) > > -extern int virtio_crypto_logtype_driver; > -#define RTE_LOGTYPE_VIRTIO_CRYPTO_DRIVER virtio_crypto_logtype_driver > +extern int virtio_logtype_driver; > > -#define VIRTIO_CRYPTO_DRV_LOG_IMPL(level, ...) \ > - RTE_LOG_LINE_PREFIX(level, VIRTIO_CRYPTO_DRIVER, "%s(): ", __func__, > __VA_ARGS__) > +#define VIRTIO_CRYPTO_DRV_LOG_IMPL(level, fmt, args...) \ > + rte_log(RTE_LOG_ ## level, virtio_logtype_driver, \ > + "DRIVER: %s(): " fmt "\n", __func__, ##args) > > #define VIRTIO_CRYPTO_DRV_LOG_INFO(fmt, ...) \ > VIRTIO_CRYPTO_DRV_LOG_IMPL(INFO, fmt, ## __VA_ARGS__) > @@ -90,4 +84,4 @@ extern int virtio_crypto_logtype_driver; > #define VIRTIO_CRYPTO_DRV_LOG_ERR(fmt, ...) \ > VIRTIO_CRYPTO_DRV_LOG_IMPL(ERR, fmt, ## __VA_ARGS__) > > -#endif /* _VIRTIO_LOGS_H_ */ > +#endif /* _VIRTIO_CRYPTO_LOGS_H_ */ > diff --git a/drivers/crypto/virtio/virtio_cryptodev.c > b/drivers/crypto/virtio/virtio_cryptodev.c > index d3db4f898e..b31e7ea0cf 100644 > --- a/drivers/crypto/virtio/virtio_cryptodev.c > +++ b/drivers/crypto/virtio/virtio_cryptodev.c > @@ -1749,8 +1749,8 @@ RTE_PMD_REGISTER_PCI(CRYPTODEV_NAME_VIRTIO_PMD, > rte_virtio_crypto_driver); > RTE_PMD_REGISTER_CRYPTO_DRIVER(virtio_crypto_drv, > rte_virtio_crypto_driver.driver, > cryptodev_virtio_driver_id); > -RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_init, init, NOTICE); > +RTE_LOG_REGISTER_SUFFIX(virtio_logtype_init, init, NOTICE); > RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_session, session, NOTICE); > RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_rx, rx, NOTICE); > RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_tx, tx, NOTICE); > -RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_driver, driver, NOTICE); > +RTE_LOG_REGISTER_SUFFIX(virtio_logtype_driver, driver, NOTICE); > diff --git a/drivers/crypto/virtio/virtqueue.h > b/drivers/crypto/virtio/virtqueue.h > index b31342940e..ccf45800c0 100644 > --- a/drivers/crypto/virtio/virtqueue.h > +++ b/drivers/crypto/virtio/virtqueue.h > @@ -15,7 +15,7 @@ > #include "virtio_cvq.h" > #include "virtio_pci.h" > #include "virtio_ring.h" > -#include "virtio_logs.h" > +#include "virtio_crypto_logs.h" > #include "virtio_crypto.h" > #include "virtio_rxtx.h" > > diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build > index 02742da5c2..6331366712 100644 > --- a/drivers/net/virtio/meson.build > +++ b/drivers/net/virtio/meson.build > @@ -22,6 +22,7 @@ sources += files( > 'virtqueue.c', > ) > deps += ['kvargs', 'bus_pci'] > +includes += include_directories('../../common/virtio') Idem, this is unneeded, as long as there is a meson dependency (like below). > > if arch_subdir == 'x86' > if cc_has_avx512 > @@ -56,5 +57,5 @@ if is_linux > 'virtio_user/vhost_user.c', > 'virtio_user/vhost_vdpa.c', > 'virtio_user/virtio_user_dev.c') > - deps += ['bus_vdev'] > + deps += ['bus_vdev', 'common_virtio'] > endif > diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c > index d9e642f412..21b0490fe7 100644 > --- a/drivers/net/virtio/virtio.c > +++ b/drivers/net/virtio/virtio.c > @@ -5,8 +5,9 @@ > > #include <unistd.h> > > +#include "virtio_net_logs.h" > + > #include "virtio.h" > -#include "virtio_logs.h" > > uint64_t > virtio_negotiate_features(struct virtio_hw *hw, uint64_t host_features) > diff --git a/drivers/net/virtio/virtio_ethdev.c > b/drivers/net/virtio/virtio_ethdev.c > index 70d4839def..491b75ec19 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -29,9 +29,10 @@ > #include <rte_cycles.h> > #include <rte_kvargs.h> > > +#include "virtio_net_logs.h" > + > #include "virtio_ethdev.h" > #include "virtio.h" > -#include "virtio_logs.h" > #include "virtqueue.h" > #include "virtio_cvq.h" > #include "virtio_rxtx.h" > diff --git a/drivers/net/virtio/virtio_net_logs.h > b/drivers/net/virtio/virtio_net_logs.h > new file mode 100644 > index 0000000000..bd5867b1fe > --- /dev/null > +++ b/drivers/net/virtio/virtio_net_logs.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2014 Intel Corporation > + */ > + > +#ifndef _VIRTIO_NET_LOGS_H_ > +#define _VIRTIO_NET_LOGS_H_ > + > +#include <inttypes.h> > + > +#include <rte_log.h> > + > +#include "virtio_logs.h" > + > +#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>") > + > +#ifdef RTE_LIBRTE_VIRTIO_DEBUG_RX > +#define PMD_RX_LOG(level, fmt, args...) \ > + RTE_LOG(level, VIRTIO_DRIVER, "%s() rx: " fmt "\n", __func__, ## args) Rebase damage. > +#else > +#define PMD_RX_LOG(level, fmt, args...) do { } while (0) > +#endif > + > +#ifdef RTE_LIBRTE_VIRTIO_DEBUG_TX > +#define PMD_TX_LOG(level, fmt, args...) \ > + RTE_LOG(level, VIRTIO_DRIVER, "%s() tx: " fmt "\n", __func__, ## args) > +#else > +#define PMD_TX_LOG(level, fmt, args...) do { } while (0) > +#endif > + > +#endif /* _VIRTIO_NET_LOGS_H_ */ -- David Marchand