Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
On Mon, Apr 26, 2021 at 11:20:32PM +0530, bkkarthik wrote:
On 21/04/26 08:04AM, Leon Romanovsky wrote:
On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote:
isapnp_proc_init() does not look at the return value from isapnp_proc_attach_device(). Check for this return value in isapnp_proc_detach_device().
Cleanup in isapnp_proc_detach_device and isapnp_proc_detach_bus() for cleanup.
Changed sprintf() to the kernel-space function scnprintf() as it returns the actual number of bytes written.
Removed unnecessary variables de, e of type 'struct proc_dir_entry' to save memory.
<...>
+static int isapnp_proc_detach_device(struct pnp_dev *dev) +{ + proc_remove(dev->procent); + dev->procent = NULL; + return 0; +} + +static int isapnp_proc_detach_bus(struct pnp_card *bus) +{ + proc_remove(bus->procdir); + return 0; +}
Please don't add one line functions that are called only once and have return value that no one care about it.
These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. Maybe those should be changed?
Probably, the CONFIG_PROC_FS around pci_proc_*() is not needed too. Thanks
On Tue, 27 Apr 2021 07:26:27 +0300, Leon Romanovsky said:
On Mon, Apr 26, 2021 at 11:20:32PM +0530, bkkarthik wrote:
These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. Maybe those should be changed?
Probably, the CONFIG_PROC_FS around pci_proc_*() is not needed too.
Will that actually build correctly if it's an embedded system or something build with CONFIG_PROC_FS=n? I'd expect that to die a horrid death while linking vmlinx due to stuff inside that #ifdef calling symbols only present with PROC_FS=m/y. In general, inline ifdef's are frowned upon, so if you come across one in the kernel source, that's probably a *big* hint that either (a) refactoring the code to eliminate an inline ifdef was just too ugly to be allowed to live or (b) you *have* to put a guard around it because you're guaranteed a build failure otherwise.
On Thu, Apr 29, 2021 at 12:31:13AM -0400, Valdis Klētnieks wrote:
On Tue, 27 Apr 2021 07:26:27 +0300, Leon Romanovsky said:
On Mon, Apr 26, 2021 at 11:20:32PM +0530, bkkarthik wrote:
These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. Maybe those should be changed?
Probably, the CONFIG_PROC_FS around pci_proc_*() is not needed too.
Will that actually build correctly if it's an embedded system or something build with CONFIG_PROC_FS=n? I'd expect that to die a horrid death while linking vmlinx due to stuff inside that #ifdef calling symbols only present with PROC_FS=m/y.
We are talking about pci_proc_detach_device() and pci_proc_detach_bus() here. They will build perfectly without CONFIG_PROC_FS. Thanks
participants (2)
-
Leon Romanovsky -
Valdis Klētnieks