[PATCH] firewire: Replace ENOSYS with appropriate error codes

[PATCH] firewire: Replace ENOSYS with appropriate error codes

[Nirbhay Sharma]

ENOSYS is reserved for "invalid syscall number" and should not be used
for other error conditions. Replace incorrect usages with more
appropriate error codes:

- In sbp2.c: Use -EOPNOTSUPP for unsupported operation (re-adding
  logical units via SCSI stack).

- In ohci.c: Use -EINVAL for invalid ISO context types in switch
  statements, and -EOPNOTSUPP for unsupported Pinnacle MovieBoard
  hardware.

- In core-cdev.c: Use -EACCES for access policy violations when
  operations are restricted to local nodes' device files.

Signed-off-by: Nirbhay Sharma <nirbhay.lkd@gmail.com>
---
 drivers/firewire/core-cdev.c | 6 +++---
 drivers/firewire/ohci.c      | 8 ++++----
 drivers/firewire/sbp2.c      | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 49dc1612c691..a62ac2f02c49 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -935,7 +935,7 @@ static int ioctl_add_descriptor(struct client *client, union ioctl_arg *arg)
 
 	/* Access policy: Allow this ioctl only on local nodes' device files. */
 	if (!client->device->is_local)
-		return -ENOSYS;
+		return -EACCES;
 
 	if (a->length > 256)
 		return -EINVAL;
@@ -1628,7 +1628,7 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
 
 	/* Access policy: Allow this ioctl only on local nodes' device files. */
 	if (!client->device->is_local)
-		return -ENOSYS;
+		return -EACCES;
 
 	e = kzalloc(sizeof(*e) + sizeof(a->data), GFP_KERNEL);
 	if (e == NULL)
@@ -1676,7 +1676,7 @@ static int ioctl_receive_phy_packets(struct client *client, union ioctl_arg *arg
 
 	/* Access policy: Allow this ioctl only on local nodes' device files. */
 	if (!client->device->is_local)
-		return -ENOSYS;
+		return -EACCES;
 
 	// NOTE: This can be without irq when we can guarantee that __fw_send_request() for local
 	// destination never runs in any type of IRQ context.
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 030aed5453a1..15aec93b42fb 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -2918,7 +2918,7 @@ static struct fw_iso_context *ohci_allocate_iso_context(struct fw_card *card,
 
 		default:
 			index = -1;
-			ret = -ENOSYS;
+			ret = -EINVAL;
 		}
 
 		if (index < 0)
@@ -3370,7 +3370,7 @@ static int ohci_queue_iso(struct fw_iso_context *base,
 	case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
 		return queue_iso_buffer_fill(ctx, packet, buffer, payload);
 	default:
-		return -ENOSYS;
+		return -EINVAL;
 	}
 }
 
@@ -3401,7 +3401,7 @@ static int ohci_flush_iso_completions(struct fw_iso_context *base)
 				flush_ir_buffer_fill(ctx);
 			break;
 		default:
-			ret = -ENOSYS;
+			ret = -EINVAL;
 		}
 
 		clear_bit_unlock(0, &ctx->flushing_completions);
@@ -3485,7 +3485,7 @@ static int pci_probe(struct pci_dev *dev,
 
 	if (dev->vendor == PCI_VENDOR_ID_PINNACLE_SYSTEMS) {
 		dev_err(&dev->dev, "Pinnacle MovieBoard is not yet supported\n");
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 	}
 
 	ohci = devres_alloc(release_ohci, sizeof(*ohci), GFP_KERNEL);
diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 1a19828114cf..2e17402466e2 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1496,7 +1496,7 @@ static int sbp2_scsi_sdev_init(struct scsi_device *sdev)
 
 	/* (Re-)Adding logical units via the SCSI stack is not supported. */
 	if (!lu)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	sdev->allow_restart = 1;
 
-- 
2.48.1

Re: [PATCH] firewire: Replace ENOSYS with appropriate error codes

[Takashi Sakamoto]

Hi,

On Mon, Nov 17, 2025 at 04:39:01PM +0530, Nirbhay Sharma wrote:
> ENOSYS is reserved for "invalid syscall number" and should not be used
> for other error conditions. Replace incorrect usages with more
> appropriate error codes:
 
Yes. The newly-written code should not use ENOSYS for cadual use, indeed.

> - In sbp2.c: Use -EOPNOTSUPP for unsupported operation (re-adding
>   logical units via SCSI stack).
> 
> - In ohci.c: Use -EINVAL for invalid ISO context types in switch
>   statements, and -EOPNOTSUPP for unsupported Pinnacle MovieBoard
>   hardware.
> 
> - In core-cdev.c: Use -EACCES for access policy violations when
>   operations are restricted to local nodes' device files.
>
> Signed-off-by: Nirbhay Sharma <nirbhay.lkd@gmail.com>
> ---
>  drivers/firewire/core-cdev.c | 6 +++---
>  drivers/firewire/ohci.c      | 8 ++++----
>  drivers/firewire/sbp2.c      | 2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
 
There is a rest to discuss when changing existing code in respect to
this topic, since it brings loss of backward-compatibility to userspace
software. In this reason, I've left them as is.

If there are any strong and specific reasons to correct them, let us
change them. Do you have such reasons? For example, Linux kernel
developer have shared the consensus and decision to ostracize such codes?


Thanks

Takashi Sakamoto

Re: [PATCH] firewire: Replace ENOSYS with appropriate error codes

[Nirbhay Sharma]

On 11/17/25 5:01 PM, Takashi Sakamoto wrote:
> Hi,
>   
> Yes. The newly-written code should not use ENOSYS for cadual use, indeed.
> 
>   
> There is a rest to discuss when changing existing code in respect to
> this topic, since it brings loss of backward-compatibility to userspace
> software. In this reason, I've left them as is.
> 
> If there are any strong and specific reasons to correct them, let us
> change them. Do you have such reasons? For example, Linux kernel
> developer have shared the consensus and decision to ostracize such codes?
> 
> 
> Thanks
> 
> Takashi Sakamoto

Hi Takashi,

Thank you for your detailed review and explanation.

You are absolutely right about the backward compatibility concern. I 
realize now that changing error codes in existing code paths could break 
userspace applications that might be checking for specific error values.

My patch was motivated by the checkpatch.pl warning and the general 
kernel policy that ENOSYS should only mean "invalid syscall number." 
BUt, I didn't fully consider the userspace ABI implications of changing 
error codes in code that's already been released.

I do not have a strong technical reason beyond code correctness to 
justify breaking backward compatibility in this case. Since these 
interfaces are already in use and userspace software may depend on the 
current behavior, the risk of breaking existing applications outweighs 
the benefit of this cleanup.

I withdraw this patch. Thank you for taking the time to explain this, 
its an important lesson about the difference between fixing issues in 
new code versus maintaining stability in existing interfaces.

Thanks,
Nirbhay