diff --git a/bcm283x/gpio.go b/bcm283x/gpio.go index ad7ad8b..61cc250 100644 --- a/bcm283x/gpio.go +++ b/bcm283x/gpio.go @@ -7,6 +7,7 @@ package bcm283x import ( "errors" "fmt" + "log" "os" "strconv" "strings" @@ -1440,28 +1441,19 @@ func (d *driverGPIO) Init() (bool, error) { } for _, a := range aliases { if err := gpioreg.RegisterAlias(a[0], a[1]); err != nil { - return true, err + // Non-fatal: if the ioctl-gpio driver already registered this name + // as a real pin, the alias is redundant. Aborting here would prevent + // the mmap-based GPIO memory from being initialised, forcing all + // operations through the ioctl fallback path — which cannot read the + // state of output pins that were set by a previous process. + log.Printf("bcm283x: skipping alias %s→%s: %v", a[0], a[1], err) } } m, err := pmem.MapGPIO() if err != nil { - // Try without /dev/gpiomem. This is the case of not running on Raspbian or - // raspbian before Jessie. This requires running as root. - var err2 error - m, err2 = pmem.Map(uint64(d.gpioBaseAddr), 4096) - var err error - if err2 != nil { - if distro.IsRaspbian() { - // Raspbian specific error code to help guide the user to troubleshoot - // the problems. - if os.IsNotExist(err) && os.IsPermission(err2) { - return true, fmt.Errorf("/dev/gpiomem wasn't found; please upgrade to Raspbian Jessie or run as root") - } - } - if os.IsPermission(err2) { - return true, fmt.Errorf("need more access, try as root: %v", err) - } + m, err = d.mapGPIOFallback(err) + if err != nil { return true, err } } @@ -1472,6 +1464,40 @@ func (d *driverGPIO) Init() (bool, error) { return true, sysfs.I2CSetSpeedHook(setSpeed) } +// mapGPIOFallback attempts to map GPIO memory via /dev/mem when /dev/gpiomem +// is unavailable. gpioErr is the error from the initial MapGPIO() attempt. +// Returns the mapped view, or an error if the fallback also fails. +func (d *driverGPIO) mapGPIOFallback(gpioErr error) (*pmem.View, error) { + m, mapErr := pmem.Map(uint64(d.gpioBaseAddr), 4096) + if mapErr != nil { + if distro.IsRaspbian() { + if os.IsNotExist(gpioErr) && os.IsPermission(mapErr) { + return nil, fmt.Errorf("/dev/gpiomem wasn't found; please upgrade to Raspbian Jessie or run as root") + } + } + if os.IsPermission(mapErr) { + // Check if /dev/gpiomem exists but has restrictive permissions. + // Raspberry Pi OS sets /dev/gpiomem to root:gpio 0660, but Ubuntu + // sets it to root:root 0600 — making it inaccessible to non-root + // users even if they are in the gpio group. A udev rule is needed: + // KERNEL=="gpiomem", GROUP="gpio", MODE="0660" + if os.IsPermission(gpioErr) { + if info, statErr := os.Stat("/dev/gpiomem"); statErr == nil { + return nil, fmt.Errorf( + "/dev/gpiomem exists (mode %v) but is not accessible, "+ + "and /dev/mem requires root; on Ubuntu, add a udev rule to grant group access: "+ + `KERNEL=="gpiomem", GROUP="gpio", MODE="0660" `+ + "(Raspberry Pi OS sets this automatically): %v", + info.Mode(), gpioErr) + } + } + return nil, fmt.Errorf("need more access, try as root: %v", gpioErr) + } + return nil, mapErr + } + return m, nil +} + func setSpeed(f physic.Frequency) error { // Writing to "/sys/module/i2c_bcm2708/parameters/baudrate" was confirmed to // not work. diff --git a/bcm283x/map_test.go b/bcm283x/map_test.go new file mode 100644 index 0000000..82ba215 --- /dev/null +++ b/bcm283x/map_test.go @@ -0,0 +1,104 @@ +// Copyright 2024 The Periph Authors. All rights reserved. +// Use of this source code is governed under the Apache License, Version 2.0 +// that can be found in the LICENSE file. + +package bcm283x + +import ( + "errors" + "os" + "strings" + "testing" + "time" + + "periph.io/x/conn/v3/gpio" + "periph.io/x/conn/v3/gpio/gpioreg" + "periph.io/x/conn/v3/physic" + "periph.io/x/conn/v3/pin" +) + +// TestMapGPIOFallbackPropagatesError verifies that when both MapGPIO() and +// Map() fail, the error from Map() is returned — not nil. This is a +// regression test for https://github.com/periph/host/issues/76 where a +// variable shadowing bug caused the error to be swallowed. +func TestMapGPIOFallbackPropagatesError(t *testing.T) { + d := &driverGPIO{gpioBaseAddr: 0x3F200000} + gpioErr := errors.New("MapGPIO: /dev/gpiomem not found") + + // mapGPIOFallback will call pmem.Map which fails on non-Linux. + _, err := d.mapGPIOFallback(gpioErr) + if err == nil { + t.Fatal("mapGPIOFallback returned nil error when Map() fails; error was swallowed") + } +} + +// TestMapGPIOFallbackPermissionError verifies that the error message includes +// the original MapGPIO error when the fallback fails with a permission error. +func TestMapGPIOFallbackPermissionError(t *testing.T) { + d := &driverGPIO{gpioBaseAddr: 0x3F200000} + gpioErr := os.ErrNotExist + + // On non-Linux, pmem.Map returns a generic "not supported" error, not a + // permission error, so this test verifies the non-permission path returns + // a non-nil error. + _, err := d.mapGPIOFallback(gpioErr) + if err == nil { + t.Fatal("mapGPIOFallback returned nil error; expected Map() failure to propagate") + } +} + +// TestAliasConflictDoesNotAbortInit verifies that when the ioctl-gpio driver +// has already registered a pin name (e.g. "PWM0_OUT"), the bcm283x driver's +// alias registration does not abort — it logs and continues. Aborting here +// would prevent gpioMemory from being initialised, forcing all GPIO operations +// through the ioctl fallback path. The ioctl path cannot read the state of +// output pins set by a previous process (the line fd is per-process), so +// restoreStartupState would always read LOW regardless of actual hardware state. +// +// Regression test for the alias conflict discovered via +// https://github.com/periph/host/issues/75 investigation. +func TestAliasConflictDoesNotAbortInit(t *testing.T) { + // Simulate the ioctl-gpio driver having registered "PWM0_OUT" as a real pin + // before the bcm283x driver runs. + const testPin = "TestAliasConflict_PWM" + if err := gpioreg.Register(&fakePin{name: testPin}); err != nil { + t.Fatalf("setup: Register fakePin: %v", err) + } + defer gpioreg.Unregister(testPin) + + // RegisterAlias should fail because the name is already a real pin. + err := gpioreg.RegisterAlias(testPin, "CLK0") + if err == nil { + t.Fatal("expected RegisterAlias to fail for an already-registered pin name") + } + if !strings.Contains(err.Error(), "pin that exists") { + t.Fatalf("unexpected error: %v", err) + } + + // The fix: the bcm283x Init loop logs this error instead of returning it. + // This test documents the scenario — the behavioural verification is that + // bcm283x-gpio appears in state.Loaded (not state.Failed) on real hardware. +} + +// fakePin implements gpio.PinIO minimally for test registration. +type fakePin struct { + name string +} + +var _ gpio.PinIO = &fakePin{} // compile-time interface check + +func (p *fakePin) String() string { return p.name } +func (p *fakePin) Name() string { return p.name } +func (p *fakePin) Number() int { return -1 } +func (p *fakePin) Function() string { return "" } +func (p *fakePin) Func() pin.Func { return "" } +func (p *fakePin) SupportedFuncs() []pin.Func { return nil } +func (p *fakePin) SetFunc(pin.Func) error { return nil } +func (p *fakePin) Halt() error { return nil } +func (p *fakePin) In(gpio.Pull, gpio.Edge) error { return nil } +func (p *fakePin) Read() gpio.Level { return false } +func (p *fakePin) WaitForEdge(time.Duration) bool { return false } +func (p *fakePin) Pull() gpio.Pull { return 0 } +func (p *fakePin) DefaultPull() gpio.Pull { return 0 } +func (p *fakePin) Out(gpio.Level) error { return nil } +func (p *fakePin) PWM(gpio.Duty, physic.Frequency) error { return nil } diff --git a/gpioioctl/gpio.go b/gpioioctl/gpio.go index c4bbcaa..6ba6b67 100644 --- a/gpioioctl/gpio.go +++ b/gpioioctl/gpio.go @@ -168,15 +168,12 @@ func (line *GPIOLine) PWM(gpio.Duty, physic.Frequency) error { return errors.New("PWM() not implemented") } -// Read the value of this line. Implements gpio.PinIn +// Read the value of this line. Implements gpio.PinIn. +// +// For output pins, this reads the currently driven value without reconfiguring +// the pin direction. The GPIO v2 chardev ioctl supports reading the value of +// output lines directly. func (line *GPIOLine) Read() gpio.Level { - if line.direction != LineInput { - err := line.In(gpio.PullUp, gpio.NoEdge) - if err != nil { - log.Println("GPIOLine.Read(): ", err) - return false - } - } line.mu.Lock() defer line.mu.Unlock() var data gpio_v2_line_values diff --git a/gpioioctl/read_test.go b/gpioioctl/read_test.go new file mode 100644 index 0000000..d538284 --- /dev/null +++ b/gpioioctl/read_test.go @@ -0,0 +1,63 @@ +// Copyright 2024 The Periph Authors. All rights reserved. +// Use of this source code is governed under the Apache License, Version 2.0 +// that can be found in the LICENSE file. + +package gpioioctl + +import ( + "testing" + + "periph.io/x/conn/v3/gpio" +) + +func init() { + if len(Chips) == 0 { + makeDummyChip() + } +} + +// TestReadOutputPinPreservesDirection verifies that calling Read() on a pin +// configured as output does not reconfigure it as input. This is a regression +// test for https://github.com/periph/host/issues/75. +// +// The bug: Read() called In(gpio.PullUp, gpio.NoEdge) on output pins, +// silently switching them from output to input with pull-up enabled. For +// power-management applications this drives output pins high — a safety hazard. +func TestReadOutputPinPreservesDirection(t *testing.T) { + line := Chips[0].Lines()[0] + + // Configure as output. On non-Linux this will fail at the ioctl level, + // but the direction field is set before the ioctl call. + line.direction = LineOutput + line.pull = gpio.PullNoChange + line.edge = gpio.NoEdge + + // Read the pin value. This should NOT change the direction. + _ = line.Read() + + if line.direction != LineOutput { + t.Errorf("Read() changed direction from Output to %s; want Output preserved", + DirectionLabels[line.direction]) + } + if line.pull != gpio.PullNoChange { + t.Errorf("Read() changed pull from PullNoChange to %s; want PullNoChange preserved", + PullLabels[line.pull]) + } +} + +// TestReadOutputPinDoesNotCallIn verifies that Read() on an output pin reads +// the driven value directly without calling In() to reconfigure the pin. +func TestReadOutputPinDoesNotCallIn(t *testing.T) { + line := Chips[0].Lines()[0] + + line.direction = LineOutput + line.pull = gpio.PullNoChange + line.edge = gpio.NoEdge + + // After Read(), direction must still be output. + _ = line.Read() + + if line.direction == LineInput { + t.Fatal("Read() reconfigured output pin as input — this drives output pins high via pull-up") + } +}