| Message ID | 20240702225845.322234-4-admiyo@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MCTP over PCC | expand |
On Tue, Jul 02, 2024 at 06:58:45PM -0400, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.amperecomputing.com> > > Implementation of network driver for > Management Control Transport Protocol(MCTP) over > Platform Communication Channel(PCC) > > DMTF DSP:0292 > > MCTP devices are specified by entries in DSDT/SDST and > reference channels specified in the PCCT. > > Communication with other devices use the PCC based > doorbell mechanism. > > Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> > --- > drivers/net/mctp/Kconfig | 13 ++ > drivers/net/mctp/Makefile | 1 + > drivers/net/mctp/mctp-pcc.c | 322 ++++++++++++++++++++++++++++++++++++ ... > diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c ... > +static struct acpi_driver mctp_pcc_driver = { > + .name = "mctp_pcc", > + .class = "Unknown", > + .ids = mctp_pcc_device_ids, > + .ops = { > + .add = mctp_pcc_driver_add, > + .remove = mctp_pcc_driver_remove, > + }, > + .owner = THIS_MODULE, Hi Adam, Perhaps net-next isn't the appropriate tree to apply this patch. But, due to [1] the owner field is not present in net-next and thus this fails to build when applied there. [1] cc85f9c05bba ("ACPI: drop redundant owner from acpi_driver") > +}; > + > +module_acpi_driver(mctp_pcc_driver); > + > +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids); > + > +MODULE_DESCRIPTION("MCTP PCC device"); > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>"); > -- > 2.34.1 > >
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc6 next-20240703]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240703-163558
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240702225845.322234-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240704/202407041311.rvCLf6kZ-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041311.rvCLf6kZ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407041311.rvCLf6kZ-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/net/mctp/mctp-pcc.c:17:
include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration
72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
| ^~~~~~~~~~~~~
In file included from include/linux/printk.h:570,
from include/asm-generic/bug.h:22,
from arch/sh/include/asm/bug.h:112,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/sh/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:79,
from include/linux/spinlock.h:56,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from drivers/net/mctp/mctp-pcc.c:7:
drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add':
drivers/net/mctp/mctp-pcc.c:207:26: error: invalid use of undefined type 'struct acpi_device'
207 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~
include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call'
273 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:207:9: note: in expansion of macro 'dev_dbg'
207 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:208:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
208 | acpi_device_hid(acpi_dev));
| ^~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call'
273 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:207:9: note: in expansion of macro 'dev_dbg'
207 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:209:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
209 | dev_handle = acpi_device_handle(acpi_dev);
| ^~~~~~~~~~~~~~~~~~
| acpi_device_dep
drivers/net/mctp/mctp-pcc.c:209:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
209 | dev_handle = acpi_device_handle(acpi_dev);
| ^
In file included from include/linux/device.h:15,
from include/linux/acpi.h:14:
drivers/net/mctp/mctp-pcc.c:213:34: error: invalid use of undefined type 'struct acpi_device'
213 | dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
| ^~
include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
drivers/net/mctp/mctp-pcc.c:213:17: note: in expansion of macro 'dev_err'
213 | dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:218:24: error: invalid use of undefined type 'struct acpi_device'
218 | dev = &acpi_dev->dev;
| ^~
drivers/net/mctp/mctp-pcc.c:264:17: error: invalid use of undefined type 'struct acpi_device'
264 | acpi_dev->driver_data = mctp_pcc_dev;
| ^~
drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_remove':
>> drivers/net/mctp/mctp-pcc.c:293:47: error: implicit declaration of function 'acpi_driver_data'; did you mean 'acpi_get_data'? [-Werror=implicit-function-declaration]
293 | struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
| ^~~~~~~~~~~~~~~~
| acpi_get_data
>> drivers/net/mctp/mctp-pcc.c:293:47: warning: initialization of 'struct mctp_pcc_ndev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
drivers/net/mctp/mctp-pcc.c: At top level:
drivers/net/mctp/mctp-pcc.c:305:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
305 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:306:10: error: 'struct acpi_driver' has no member named 'name'
306 | .name = "mctp_pcc",
| ^~~~
drivers/net/mctp/mctp-pcc.c:306:17: warning: excess elements in struct initializer
306 | .name = "mctp_pcc",
| ^~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:306:17: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:307:10: error: 'struct acpi_driver' has no member named 'class'
307 | .class = "Unknown",
| ^~~~~
drivers/net/mctp/mctp-pcc.c:307:18: warning: excess elements in struct initializer
307 | .class = "Unknown",
| ^~~~~~~~~
drivers/net/mctp/mctp-pcc.c:307:18: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:308:10: error: 'struct acpi_driver' has no member named 'ids'
308 | .ids = mctp_pcc_device_ids,
| ^~~
drivers/net/mctp/mctp-pcc.c:308:16: warning: excess elements in struct initializer
308 | .ids = mctp_pcc_device_ids,
| ^~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:308:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:309:10: error: 'struct acpi_driver' has no member named 'ops'
309 | .ops = {
| ^~~
drivers/net/mctp/mctp-pcc.c:309:16: error: extra brace group at end of initializer
309 | .ops = {
| ^
drivers/net/mctp/mctp-pcc.c:309:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:309:16: warning: excess elements in struct initializer
drivers/net/mctp/mctp-pcc.c:309:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:313:10: error: 'struct acpi_driver' has no member named 'owner'
313 | .owner = THIS_MODULE,
| ^~~~~
In file included from arch/sh/include/asm/cache.h:12,
from include/linux/cache.h:6,
from include/linux/slab.h:15:
include/linux/init.h:180:21: warning: excess elements in struct initializer
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:313:18: note: in expansion of macro 'THIS_MODULE'
313 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
include/linux/init.h:180:21: note: (near initialization for 'mctp_pcc_driver')
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:313:18: note: in expansion of macro 'THIS_MODULE'
313 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:316:1: warning: data definition has no type or storage class
316 | module_acpi_driver(mctp_pcc_driver);
| ^~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:316:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Werror=implicit-int]
drivers/net/mctp/mctp-pcc.c:316:1: warning: parameter names (without types) in function declaration
drivers/net/mctp/mctp-pcc.c:305:27: error: storage size of 'mctp_pcc_driver' isn't known
305 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:305:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]
cc1: some warnings being treated as errors
vim +293 drivers/net/mctp/mctp-pcc.c
290
291 static void mctp_pcc_driver_remove(struct acpi_device *adev)
292 {
> 293 struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
294
295 pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
296 pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
297 mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
298 }
299
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc6 next-20240703]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240703-163558
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240702225845.322234-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240704/202407041340.ve4NQPsF-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 326ba38a991250a8587a399a260b0f7af2c9166a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041340.ve4NQPsF-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407041340.ve4NQPsF-lkp@intel.com/
All errors (new ones prefixed by >>):
| ^
In file included from drivers/net/mctp/mctp-pcc.c:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/net/mctp/mctp-pcc.c:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/net/mctp/mctp-pcc.c:17:
include/acpi/acpi_drivers.h:72:43: warning: declaration of 'struct acpi_pci_root' will not be visible outside of this function [-Wvisibility]
72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
| ^
drivers/net/mctp/mctp-pcc.c:207:19: error: incomplete definition of type 'struct acpi_device'
207 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ~~~~~~~~^
include/linux/dev_printk.h:165:18: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:274:7: note: expanded from macro 'dynamic_dev_dbg'
274 | dev, fmt, ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
248 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
793 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:208:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
208 | acpi_device_hid(acpi_dev));
| ^
drivers/net/mctp/mctp-pcc.c:208:3: note: did you mean 'acpi_device_dep'?
include/acpi/acpi_bus.h:41:6: note: 'acpi_device_dep' declared here
41 | bool acpi_device_dep(acpi_handle target, acpi_handle match);
| ^
drivers/net/mctp/mctp-pcc.c:209:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
209 | dev_handle = acpi_device_handle(acpi_dev);
| ^
drivers/net/mctp/mctp-pcc.c:209:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
209 | dev_handle = acpi_device_handle(acpi_dev);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:213:20: error: incomplete definition of type 'struct acpi_device'
213 | dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
| ~~~~~~~~^
include/linux/dev_printk.h:154:44: note: expanded from macro 'dev_err'
154 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
793 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:218:17: error: incomplete definition of type 'struct acpi_device'
218 | dev = &acpi_dev->dev;
| ~~~~~~~~^
include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
793 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:264:10: error: incomplete definition of type 'struct acpi_device'
264 | acpi_dev->driver_data = mctp_pcc_dev;
| ~~~~~~~~^
include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
793 | struct acpi_device;
| ^
>> drivers/net/mctp/mctp-pcc.c:293:40: error: call to undeclared function 'acpi_driver_data'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
293 | struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
| ^
>> drivers/net/mctp/mctp-pcc.c:293:24: error: incompatible integer to pointer conversion initializing 'struct mctp_pcc_ndev *' with an expression of type 'int' [-Wint-conversion]
293 | struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
| ^ ~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:305:27: error: variable has incomplete type 'struct acpi_driver'
305 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:305:15: note: forward declaration of 'struct acpi_driver'
305 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:316:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
316 | module_acpi_driver(mctp_pcc_driver);
| ^
| int
drivers/net/mctp/mctp-pcc.c:316:20: error: a parameter list without types is only allowed in a function definition
316 | module_acpi_driver(mctp_pcc_driver);
| ^
8 warnings and 12 errors generated.
vim +/acpi_driver_data +293 drivers/net/mctp/mctp-pcc.c
290
291 static void mctp_pcc_driver_remove(struct acpi_device *adev)
292 {
> 293 struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
294
295 pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
296 pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
297 mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
298 }
299
Hi Adam, Still some minor things, and locking query, inline. You'll want to address the kernel-test-robot failure too; it looks like the config it's hitting ends up without the acpi definitions available; possibly CONFIG_ACPI is not set. Maybe you need a depends instead of a select? > diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig > index ce9d2d2ccf3b..ff4effd8e99c 100644 > --- a/drivers/net/mctp/Kconfig > +++ b/drivers/net/mctp/Kconfig > @@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C > A MCTP protocol network device is created for each I3C bus > having a "mctp-controller" devicetree property. > > +config MCTP_TRANSPORT_PCC > + tristate "MCTP PCC transport" Nit: you have two spaces there. > +struct mctp_pcc_hdr { > + u32 signature; > + u32 flags; > + u32 length; > + char mctp_signature[4]; > +}; I see you've added the __le annotations that I mentioned, but to a different patch in the series. Was that intentional? > + > +struct mctp_pcc_hw_addr { > + u32 inbox_index; > + u32 outbox_index; > +}; > + > +/* The netdev structure. One of these per PCC adapter. */ > +struct mctp_pcc_ndev { > + struct list_head next; This is now unused. > + /* spinlock to serialize access to pcc buffer and registers*/ > + spinlock_t lock; The addition of the comment is good, but I'm still not clear on what data this lock is serialising access to. Does "pcc buffer" refer to both the pcc_comm_inbox_addr and pcc_comm_outbox_addr buffer? If so, you may be missing locking on the client_rx_callback. And what are the "registers" referring to here? everything seems to be accessed through the mbox interface. Does that require serialisation? (if you can list actual struct members that are only accessed under the lock, that may make things more clear - but it can be a bit of a balance in not going *too* overboard with the description!) > + struct mctp_dev mdev; > + struct acpi_device *acpi_device; > + struct pcc_mbox_chan *in_chan; > + struct pcc_mbox_chan *out_chan; > + struct mbox_client outbox_client; > + struct mbox_client inbox_client; > + void __iomem *pcc_comm_inbox_addr; > + void __iomem *pcc_comm_outbox_addr; > + struct mctp_pcc_hw_addr hw_addr; > +}; > + > +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer) > +{ > + struct mctp_pcc_ndev *mctp_pcc_dev; > + struct mctp_pcc_hdr mctp_pcc_hdr; > + struct mctp_skb_cb *cb; > + struct sk_buff *skb; > + void *skb_buf; > + u32 data_len; > + > + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); > + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr, > + sizeof(struct mctp_pcc_hdr)); > + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH; > + > + if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) { > + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; > + return; > + } Shouldn't this be comparing on the currently-set MTU, rather than the max? [in either case, this assumes that we want to enforce RX packets to be within the transmit limit, which may be reasonable, but maybe not strictly necessary] > + > + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len); > + if (!skb) { > + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; > + return; > + } > + mctp_pcc_dev->mdev.dev->stats.rx_packets++; > + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len; > + skb->protocol = htons(ETH_P_MCTP); > + skb_buf = skb_put(skb, data_len); > + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len); > + > + skb_reset_mac_header(skb); > + skb_pull(skb, sizeof(struct mctp_pcc_hdr)); > + skb_reset_network_header(skb); > + cb = __mctp_cb(skb); > + cb->halen = 0; > + netif_rx(skb); > +} > + > +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct mctp_pcc_hdr pcc_header; > + struct mctp_pcc_ndev *mpnd; > + void __iomem *buffer; > + unsigned long flags; > + > + ndev->stats.tx_bytes += skb->len; > + ndev->stats.tx_packets++; > + mpnd = netdev_priv(ndev); > + > + spin_lock_irqsave(&mpnd->lock, flags); > + buffer = mpnd->pcc_comm_outbox_addr; > + pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index; > + pcc_header.flags = PCC_HEADER_FLAGS; > + memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); > + pcc_header.length = skb->len + SIGNATURE_LENGTH; Are there any constraints on this length? > + memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr)); > + memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len); > + mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, > + NULL); > + spin_unlock_irqrestore(&mpnd->lock, flags); > + > + dev_consume_skb_any(skb); > + return NETDEV_TX_OK; > +} > + > +static void > +mctp_pcc_net_stats(struct net_device *net_dev, > + struct rtnl_link_stats64 *stats) > +{ > + struct mctp_pcc_ndev *mpnd; > + > + mpnd = netdev_priv(net_dev); > + stats->rx_errors = 0; > + stats->rx_packets = mpnd->mdev.dev->stats.rx_packets; > + stats->tx_packets = mpnd->mdev.dev->stats.tx_packets; > + stats->rx_dropped = 0; > + stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes; > + stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes; Isn't mpnd->mdev.dev just net_dev? Should we be doing this (as well as the stats updates) with mpnd->lock held? > +static void mctp_pcc_driver_remove(struct acpi_device *adev) > +{ > + struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev); > + > + pcc_mbox_free_channel(mctp_pcc_ndev->out_chan); > + pcc_mbox_free_channel(mctp_pcc_ndev->in_chan); > + mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev); > +} Nice! Cheers, Jeremy
On 7/4/24 06:23, Jeremy Kerr wrote: >> +struct mctp_pcc_hdr { >> + u32 signature; >> + u32 flags; >> + u32 length; >> + char mctp_signature[4]; >> +}; > I see you've added the __le annotations that I mentioned, but to a > different patch in the series. Was that intentional? Yes. Since the header is used by the Mailbox implementation now, in order to check the ACK flag. It made sense to move the structure to a common location, but the change was based on your last review.
diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig index ce9d2d2ccf3b..ff4effd8e99c 100644 --- a/drivers/net/mctp/Kconfig +++ b/drivers/net/mctp/Kconfig @@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C A MCTP protocol network device is created for each I3C bus having a "mctp-controller" devicetree property. +config MCTP_TRANSPORT_PCC + tristate "MCTP PCC transport" + select ACPI + help + Provides a driver to access MCTP devices over PCC transport, + A MCTP protocol network device is created via ACPI for each + entry in the DST/SDST that matches the identifier. The Platform + commuinucation channels are selected from the corresponding + entries in the PCCT. + + Say y here if you need to connect to MCTP endpoints over PCC. To + compile as a module, use m; the module will be called mctp-pcc. + endmenu endif diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile index e1cb99ced54a..492a9e47638f 100644 --- a/drivers/net/mctp/Makefile +++ b/drivers/net/mctp/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c new file mode 100644 index 000000000000..50ebad0e0cbe --- /dev/null +++ b/drivers/net/mctp/mctp-pcc.c @@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * mctp-pcc.c - Driver for MCTP over PCC. + * Copyright (c) 2024, Ampere Computing LLC + */ + +#include <linux/acpi.h> +#include <linux/if_arp.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/platform_device.h> +#include <linux/string.h> + +#include <acpi/acpi_bus.h> +#include <acpi/acpi_drivers.h> +#include <acpi/acrestyp.h> +#include <acpi/actbl.h> +#include <net/mctp.h> +#include <net/mctpdevice.h> +#include <acpi/pcc.h> + +#define MCTP_PAYLOAD_LENGTH 256 +#define MCTP_CMD_LENGTH 4 +#define MCTP_PCC_VERSION 0x1 /* DSP0253 defines a single version: 1 */ +#define MCTP_SIGNATURE "MCTP" +#define SIGNATURE_LENGTH 4 +#define MCTP_HEADER_LENGTH 12 +#define MCTP_MIN_MTU 68 +#define PCC_MAGIC 0x50434300 +#define PCC_HEADER_FLAG_REQ_INT 0x1 +#define PCC_HEADER_FLAGS PCC_HEADER_FLAG_REQ_INT +#define PCC_DWORD_TYPE 0x0c +#define PCC_ACK_FLAG_MASK 0x1 + +struct mctp_pcc_hdr { + u32 signature; + u32 flags; + u32 length; + char mctp_signature[4]; +}; + +struct mctp_pcc_hw_addr { + u32 inbox_index; + u32 outbox_index; +}; + +/* The netdev structure. One of these per PCC adapter. */ +struct mctp_pcc_ndev { + struct list_head next; + /* spinlock to serialize access to pcc buffer and registers*/ + spinlock_t lock; + struct mctp_dev mdev; + struct acpi_device *acpi_device; + struct pcc_mbox_chan *in_chan; + struct pcc_mbox_chan *out_chan; + struct mbox_client outbox_client; + struct mbox_client inbox_client; + void __iomem *pcc_comm_inbox_addr; + void __iomem *pcc_comm_outbox_addr; + struct mctp_pcc_hw_addr hw_addr; +}; + +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer) +{ + struct mctp_pcc_ndev *mctp_pcc_dev; + struct mctp_pcc_hdr mctp_pcc_hdr; + struct mctp_skb_cb *cb; + struct sk_buff *skb; + void *skb_buf; + u32 data_len; + + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr, + sizeof(struct mctp_pcc_hdr)); + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH; + + if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) { + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; + return; + } + + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len); + if (!skb) { + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; + return; + } + mctp_pcc_dev->mdev.dev->stats.rx_packets++; + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len; + skb->protocol = htons(ETH_P_MCTP); + skb_buf = skb_put(skb, data_len); + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len); + + skb_reset_mac_header(skb); + skb_pull(skb, sizeof(struct mctp_pcc_hdr)); + skb_reset_network_header(skb); + cb = __mctp_cb(skb); + cb->halen = 0; + netif_rx(skb); +} + +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) +{ + struct mctp_pcc_hdr pcc_header; + struct mctp_pcc_ndev *mpnd; + void __iomem *buffer; + unsigned long flags; + + ndev->stats.tx_bytes += skb->len; + ndev->stats.tx_packets++; + mpnd = netdev_priv(ndev); + + spin_lock_irqsave(&mpnd->lock, flags); + buffer = mpnd->pcc_comm_outbox_addr; + pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index; + pcc_header.flags = PCC_HEADER_FLAGS; + memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); + pcc_header.length = skb->len + SIGNATURE_LENGTH; + memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr)); + memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len); + mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, + NULL); + spin_unlock_irqrestore(&mpnd->lock, flags); + + dev_consume_skb_any(skb); + return NETDEV_TX_OK; +} + +static void +mctp_pcc_net_stats(struct net_device *net_dev, + struct rtnl_link_stats64 *stats) +{ + struct mctp_pcc_ndev *mpnd; + + mpnd = netdev_priv(net_dev); + stats->rx_errors = 0; + stats->rx_packets = mpnd->mdev.dev->stats.rx_packets; + stats->tx_packets = mpnd->mdev.dev->stats.tx_packets; + stats->rx_dropped = 0; + stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes; + stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes; +} + +static const struct net_device_ops mctp_pcc_netdev_ops = { + .ndo_start_xmit = mctp_pcc_tx, + .ndo_get_stats64 = mctp_pcc_net_stats, +}; + +static void mctp_pcc_setup(struct net_device *ndev) +{ + ndev->type = ARPHRD_MCTP; + ndev->hard_header_len = 0; + ndev->addr_len = 0; + ndev->tx_queue_len = 0; + ndev->flags = IFF_NOARP; + ndev->netdev_ops = &mctp_pcc_netdev_ops; + ndev->needs_free_netdev = true; +} + +struct lookup_context { + int index; + u32 inbox_index; + u32 outbox_index; +}; + +static acpi_status lookup_pcct_indices(struct acpi_resource *ares, + void *context) +{ + struct acpi_resource_address32 *addr; + struct lookup_context *luc = context; + + switch (ares->type) { + case PCC_DWORD_TYPE: + break; + default: + return AE_OK; + } + + addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data); + switch (luc->index) { + case 0: + luc->outbox_index = addr[0].address.minimum; + break; + case 1: + luc->inbox_index = addr[0].address.minimum; + break; + } + luc->index++; + return AE_OK; +} + +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev) +{ + struct lookup_context context = {0, 0, 0}; + struct mctp_pcc_ndev *mctp_pcc_dev; + struct net_device *ndev; + acpi_handle dev_handle; + acpi_status status; + struct device *dev; + int mctp_pcc_mtu; + int outbox_index; + int inbox_index; + char name[32]; + int rc; + + dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n", + acpi_device_hid(acpi_dev)); + dev_handle = acpi_device_handle(acpi_dev); + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, + &context); + if (!ACPI_SUCCESS(status)) { + dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS"); + return -EINVAL; + } + inbox_index = context.inbox_index; + outbox_index = context.outbox_index; + dev = &acpi_dev->dev; + + snprintf(name, sizeof(name), "mctpipcc%d", inbox_index); + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, + mctp_pcc_setup); + if (!ndev) + return -ENOMEM; + mctp_pcc_dev = netdev_priv(ndev); + INIT_LIST_HEAD(&mctp_pcc_dev->next); + spin_lock_init(&mctp_pcc_dev->lock); + + mctp_pcc_dev->hw_addr.inbox_index = inbox_index; + mctp_pcc_dev->hw_addr.outbox_index = outbox_index; + mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback; + mctp_pcc_dev->out_chan = + pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client, + outbox_index); + if (IS_ERR(mctp_pcc_dev->out_chan)) { + rc = PTR_ERR(mctp_pcc_dev->out_chan); + goto free_netdev; + } + mctp_pcc_dev->in_chan = + pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client, + inbox_index); + if (IS_ERR(mctp_pcc_dev->in_chan)) { + rc = PTR_ERR(mctp_pcc_dev->in_chan); + goto cleanup_out_channel; + } + mctp_pcc_dev->pcc_comm_inbox_addr = + devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr, + mctp_pcc_dev->in_chan->shmem_size); + if (!mctp_pcc_dev->pcc_comm_inbox_addr) { + rc = -EINVAL; + goto cleanup_in_channel; + } + mctp_pcc_dev->pcc_comm_outbox_addr = + devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr, + mctp_pcc_dev->out_chan->shmem_size); + if (!mctp_pcc_dev->pcc_comm_outbox_addr) { + rc = -EINVAL; + goto cleanup_in_channel; + } + mctp_pcc_dev->acpi_device = acpi_dev; + mctp_pcc_dev->inbox_client.dev = dev; + mctp_pcc_dev->outbox_client.dev = dev; + mctp_pcc_dev->mdev.dev = ndev; + acpi_dev->driver_data = mctp_pcc_dev; + + /* There is no clean way to pass the MTU + * to the callback function used for registration, + * so set the values ahead of time. + */ + mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size - + sizeof(struct mctp_pcc_hdr); + ndev->mtu = MCTP_MIN_MTU; + ndev->max_mtu = mctp_pcc_mtu; + ndev->min_mtu = MCTP_MIN_MTU; + + rc = register_netdev(ndev); + if (rc) + goto cleanup_in_channel; + return 0; + +cleanup_in_channel: + pcc_mbox_free_channel(mctp_pcc_dev->in_chan); +cleanup_out_channel: + pcc_mbox_free_channel(mctp_pcc_dev->out_chan); +free_netdev: + unregister_netdev(ndev); + free_netdev(ndev); + return rc; +} + +static void mctp_pcc_driver_remove(struct acpi_device *adev) +{ + struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev); + + pcc_mbox_free_channel(mctp_pcc_ndev->out_chan); + pcc_mbox_free_channel(mctp_pcc_ndev->in_chan); + mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev); +} + +static const struct acpi_device_id mctp_pcc_device_ids[] = { + { "DMT0001", 0}, + { "", 0}, +}; + +static struct acpi_driver mctp_pcc_driver = { + .name = "mctp_pcc", + .class = "Unknown", + .ids = mctp_pcc_device_ids, + .ops = { + .add = mctp_pcc_driver_add, + .remove = mctp_pcc_driver_remove, + }, + .owner = THIS_MODULE, +}; + +module_acpi_driver(mctp_pcc_driver); + +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids); + +MODULE_DESCRIPTION("MCTP PCC device"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");