Re: OF_getpropstr()
> Date: Sat, 8 Apr 2023 13:29:25 +1000
> From: David Gwynne
>
> turns out OF_getprop is like memcpy, but sometimes you want something
> like strlcpy. this is what OF_getpropstr aims to provide.
>
> i know openfirm.h is used on other archs that don't use fdt as their
> backend, but i figure we can port this wrapper over to them as need
> demands.
>
> ok?
Going to continue to point out the redundant parentheses in your diffs!
So a few small nits.
With those fixed ok kettenis@
> Index: fdt.c
> ===
> RCS file: /cvs/src/sys/dev/ofw/fdt.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 fdt.c
> --- fdt.c 19 Sep 2022 16:12:19 - 1.33
> +++ fdt.c 8 Apr 2023 03:16:39 -
> @@ -980,6 +980,18 @@ OF_getprop(int handle, char *prop, void
> }
>
> int
> +OF_getpropstr(int handle, char *prop, char *buf, int buflen)
Can you move this function to after OF_getpropint64array() to keep
them in alphabetical order?
> +{
> + int len;
> +
> + len = OF_getprop(handle, prop, buf, buflen);
> + if (buflen > 0)
> + buf[min(len, buflen - 1)] = '\0';
> +
> + return (len);
return len;
> +}
> +
> +int
> OF_getpropbool(int handle, char *prop)
> {
> void *node = (char *)tree.header + handle;
> Index: openfirm.h
> ===
> RCS file: /cvs/src/sys/dev/ofw/openfirm.h,v
> retrieving revision 1.18
> diff -u -p -r1.18 openfirm.h
> --- openfirm.h6 May 2021 19:45:16 - 1.18
> +++ openfirm.h8 Apr 2023 03:16:39 -
> @@ -50,6 +50,7 @@ int OF_parent(int phandle);
> int OF_instance_to_package(int ihandle);
> int OF_getproplen(int handle, char *prop);
> int OF_getprop(int handle, char *prop, void *buf, int buflen);
> +int OF_getpropstr(int handle, char *prop, char *buf, int buflen);
Same here, move the proptype after OF_getpropint64array().
> int OF_getpropbool(int handle, char *);
> uint32_t OF_getpropint(int handle, char *, uint32_t);
> int OF_getpropintarray(int, char *, uint32_t *, int);
>
>
Re: OF_getpropstr()
Mark Kettenis wrote:
> > +{
> > + int len;
> > +
> > + len = OF_getprop(handle, prop, buf, buflen);
> > + if (buflen > 0)
> > + buf[min(len, buflen - 1)] = '\0';
> > +
> > + return (len);
>
I've mailed dlg seperately, but will raise it here also.
If buflen is 0, then why call OF_getprop at all? I doubt this situation
occurs, but you want to protect against it, ok
Maybe in the end if looks like this:
int len = 0;
if (buflen > 0) {
len = OF_getprop(handle, prop, buf, buflen - 1);
buf[min(len, buflen - 1)] = '\0';
}
return (len);
OF_getprop() is now being called with buflen -1, which can avoid one
extra character of processing effort for a long input string.
Re: acpithinkpad: don't setup non-existent temp sensors
On Fri, 07 Apr 2023 at 19:00:43 +, Klemens Nanni wrote:
> > + sc->sc_ntempsens = 0;
> > + for (i = 0; i < THINKPAD_NSENSORS - THINKPAD_SENSOR_TMP0; i++) {
> > + if (thinkpad_get_temp(sc, i, &tmp) != 0)
> > + break;
>
> doesn't this mean, that legit sensors which happen to fail this read
> upon attach won't ever be visible at runtime?
>
> I have no idea how reliable those sensors are, but given that the code
> handles spurious failure, this seems not too far fetched.
The sensor nodes (TMP0, TMP1, etc.) are hard-coded into the DSDT, so
the EC failing to read the actual sensor should still allow it to be
found.
> > - /* On version 2 and newer, let *drm or acpivout control brightness */
> > - if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 &&
> > - (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> > - 0, NULL, &sc->sc_brightness) == 0)) {
> > + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> > + 0, NULL, &sc->sc_brightness) == 0) {
> > ws_get_param = thinkpad_get_param;
> > ws_set_param = thinkpad_set_param;
> > }
>
> Is this an unrelated diff that snuck in?
Yes, here is a new version without that chunk.
> > + if (aml_evalinteger(sc->sc_acpi, sc->sc_ec->sc_devnode, sname, 0, 0,
> > + temp) != 0)
> > + return (1);
>
> Looks liks aml_evalnode(9) only returns 0/success or
> ACPI_E_BADVALUE/failure anyway, so there's no way to distinguish between
> nonexistent sensors and existent but failing sensors, right?
Correct, this is just reporting whether the node exists and can be
read, not whether the sensor is broken or reporting bogus values.
Index: acpithinkpad.c
===
RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
retrieving revision 1.70
diff -u -p -u -p -r1.70 acpithinkpad.c
--- acpithinkpad.c 6 Apr 2022 18:59:27 - 1.70
+++ acpithinkpad.c 8 Apr 2023 15:03:09 -
@@ -112,11 +112,10 @@
#defineTHINKPAD_TABLET_SCREEN_CHANGED 0x60c0
#defineTHINKPAD_SWITCH_WIRELESS0x7000
-#define THINKPAD_NSENSORS 10
-#define THINKPAD_NTEMPSENSORS 8
-
-#define THINKPAD_SENSOR_FANRPM THINKPAD_NTEMPSENSORS
-#define THINKPAD_SENSOR_PORTREPL THINKPAD_NTEMPSENSORS + 1
+#define THINKPAD_SENSOR_FANRPM 0
+#define THINKPAD_SENSOR_PORTREPL 1
+#define THINKPAD_SENSOR_TMP0 2
+#define THINKPAD_NSENSORS 10
#define THINKPAD_ECOFFSET_VOLUME 0x30
#define THINKPAD_ECOFFSET_VOLUME_MUTE_MASK 0x40
@@ -140,6 +139,7 @@ struct acpithinkpad_softc {
struct ksensor sc_sens[THINKPAD_NSENSORS];
struct ksensordevsc_sensdev;
+ int sc_ntempsens;
uint64_t sc_hkey_version;
@@ -178,6 +178,7 @@ int thinkpad_get_brightness(struct acpit
intthinkpad_set_brightness(void *, int);
intthinkpad_get_param(struct wsdisplay_param *);
intthinkpad_set_param(struct wsdisplay_param *);
+intthinkpad_get_temp(struct acpithinkpad_softc *, int, int64_t *);
voidthinkpad_sensor_attach(struct acpithinkpad_softc *sc);
voidthinkpad_sensor_refresh(void *);
@@ -228,6 +229,7 @@ thinkpad_match(struct device *parent, vo
void
thinkpad_sensor_attach(struct acpithinkpad_softc *sc)
{
+ int64_t tmp;
int i;
if (sc->sc_acpi->sc_ec == NULL)
@@ -237,10 +239,16 @@ thinkpad_sensor_attach(struct acpithinkp
/* Add temperature probes */
strlcpy(sc->sc_sensdev.xname, DEVNAME(sc),
sizeof(sc->sc_sensdev.xname));
- for (i=0; isc_sens[i].type = SENSOR_TEMP;
- sensor_attach(&sc->sc_sensdev, &sc->sc_sens[i]);
- }
+ sc->sc_ntempsens = 0;
+ for (i = 0; i < THINKPAD_NSENSORS - THINKPAD_SENSOR_TMP0; i++) {
+ if (thinkpad_get_temp(sc, i, &tmp) != 0)
+ break;
+
+ sc->sc_sens[THINKPAD_SENSOR_TMP0 + i].type = SENSOR_TEMP;
+ sensor_attach(&sc->sc_sensdev,
+ &sc->sc_sens[THINKPAD_SENSOR_TMP0 + i]);
+ sc->sc_ntempsens++;
+ }
/* Add fan probe */
sc->sc_sens[THINKPAD_SENSOR_FANRPM].type = SENSOR_FANRPM;
@@ -262,17 +270,19 @@ thinkpad_sensor_refresh(void *arg)
struct acpithinkpad_softc *sc = arg;
uint8_t lo, hi, i;
int64_t tmp;
- char sname[5];
/* Refresh sensor readings */
- for (i=0; isc_acpi, sc->sc_ec->sc_devnode,
- sname, 0, 0, &tmp);
- sc->sc_sens[i].value = (tmp * 100) + 27315;
- if (tmp > 127 || tmp < -127)
- sc->sc_sens[i].flags = SENSOR_FINVALID;
- }
+ for (i = 0; i < sc->sc_ntempsens; i++) {
+ if (thinkpad_get_temp(sc, i, &tmp) != 0) {
+ sc->sc_sens[i].flags = SENSOR_FINVALID;
+ continue;
+
Re: OF_getpropstr()
On Sat, 08 Apr 2023 08:48:31 -0600, "Theo de Raadt" wrote:
> Mark Kettenis wrote:
>
> > > +{
> > > + int len;
> > > +
> > > + len = OF_getprop(handle, prop, buf, buflen);
> > > + if (buflen > 0)
> > > + buf[min(len, buflen - 1)] = '\0';
> > > +
> > > + return (len);
> >
>
> I've mailed dlg seperately, but will raise it here also.
>
> If buflen is 0, then why call OF_getprop at all? I doubt this situation
> occurs, but you want to protect against it, ok
>
> Maybe in the end if looks like this:
>
> int len = 0;
> if (buflen > 0) {
> len = OF_getprop(handle, prop, buf, buflen - 1);
> buf[min(len, buflen - 1)] = '\0';
> }
> return (len);
>
> OF_getprop() is now being called with buflen -1, which can avoid one
> extra character of processing effort for a long input string.
I think that will be wrong for the "name" property. From
sys/dev/ofw/fdt.c:OF_getprop
if (len < 0 && strcmp(prop, "name") == 0) {
data = fdt_node_name(node);
if (data) {
len = strlcpy(buf, data, buflen);
...
So passing in buflen is probably correct.
- todd
Re: acpithinkpad: don't setup non-existent temp sensors
On Sat, Apr 08, 2023 at 10:09:11AM -0500, joshua stein wrote: > The sensor nodes (TMP0, TMP1, etc.) are hard-coded into the DSDT, so > the EC failing to read the actual sensor should still allow it to be > found. Understood, thanks. > Yes, here is a new version without that chunk. Works for me, looks sane, OK kn
mvsw phy-mode support
On the Turris Omnia, the host can't transmit over the interface linked to
the switch unless mvsw applies phy-mode settings to the port on its side,
specifically the rgmii delay settings.
ok?
Index: mvsw.c
===
RCS file: /cvs/src/sys/dev/fdt/mvsw.c,v
retrieving revision 1.5
diff -u -p -r1.5 mvsw.c
--- mvsw.c 6 Apr 2022 18:59:28 - 1.5
+++ mvsw.c 9 Apr 2023 04:19:21 -
@@ -52,6 +52,10 @@
#define MVSW_PORT(x) (0x10 + (x))
#define MVSW_G20x1c
+#define MVSW_PORT_MAC_CTL 0x01
+#define MVSW_PORT_MAC_CTL_RGMII_RXID 0x8000
+#define MVSW_PORT_MAC_CTL_RGMII_TXID 0x4000
+#define MVSW_PORT_MAC_CTL_RGMII_MASK 0xc000
#define MVSW_PORT_SWITCHID 0x03
#define MVSW_PORT_SWITCHID_PROD_MASK 0xfff0
#define MVSW_PORT_SWITCHID_PROD_88E6141 0x3400
@@ -70,6 +74,17 @@
/* XXX #include */
#define MDIO_MMD_PHYXS 4
+const struct {
+const char *name;
+uint16_tmac_ctl;
+} mvsw_phy_modes[] = {
+{ "rgmii", 0 },
+{ "rgmii-id", MVSW_PORT_MAC_CTL_RGMII_TXID |
+MVSW_PORT_MAC_CTL_RGMII_RXID },
+{ "rgmii-rxid", MVSW_PORT_MAC_CTL_RGMII_RXID },
+{ "rgmii-txid", MVSW_PORT_MAC_CTL_RGMII_TXID }
+};
+
struct mvsw_softc {
struct device sc_dev;
@@ -310,12 +325,27 @@ mvsw_serdes_write(struct mvsw_softc *sc,
void
mvsw_port_enable(struct mvsw_softc *sc, int node)
{
+ char phy_mode[16] = { 0 };
uint16_t val;
- int port;
+ int port, i;
port = OF_getpropint(node, "reg", -1);
if (port == -1)
return;
+
+ OF_getprop(node, "phy-mode", phy_mode, sizeof(phy_mode));
+ for (i = 0; i < nitems(mvsw_phy_modes); i++) {
+ if (strcmp(phy_mode, mvsw_phy_modes[i].name) == 0) {
+ val = mvsw_smi_read(sc, MVSW_PORT(port),
+ MVSW_PORT_MAC_CTL);
+ val &= ~MVSW_PORT_MAC_CTL_RGMII_MASK;
+ val |= mvsw_phy_modes[i].mac_ctl;
+ mvsw_smi_write(sc, MVSW_PORT(port),
+ MVSW_PORT_MAC_CTL, val);
+
+ break;
+ }
+ }
/* Enable port. */
val = mvsw_smi_read(sc, MVSW_PORT(port), MVSW_PORT_CTRL);
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > Hi, > > > > This is work in progress. I have to think if the flags to kdump I'm > > introducing should be two or a single one. > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > with option D it dumps its state to a malloc.out file at exit. This > > state can be used to find leaks amongst other things. > > > > This is not ideal for pledged processes, as they often have no way to > > write files. > > > > This changes malloc to use utrace(2) for that. > > > > As kdump has no nice way to show those lines without all extras it > > normally shows, so add two options to it to just show the lines. > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > Run : > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > ... > > $ kdump -hu > > > > Feedback appreciated. I can't really comment on malloc(3) stuff, but I agree that utrace(2) is a good way to get information outside a pledged process. I tend to think it is safe to use it, as the pledged process need cooperation from outside to exfiltrate informations (a process with permission to call ktrace(2) on this pid). Please note it is a somehow generic problem: at least profiled processes would also get advantage of using it. Regarding kdump options, I think that -u option should implies -h (no header). Does it would make sens to considere a process using utrace(2) with several interleaved records for different sources ? A process with MALLOC_OPTIONS=D and profiling enabled for example ? An (another) option on kdump to filter on utrace label would be useful in such case, or have -u mandate a label to filter on. $ MALLOC_OPTIONS=D ktrace -tu your_program $ kdump -u mallocdumpline and for profiling: $ kdump -u profil > gmon.out $ gprof your_program gmon.out Thanks. -- Sebastien Marie
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > Hi, > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > introducing should be two or a single one. > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > > with option D it dumps its state to a malloc.out file at exit. This > > > state can be used to find leaks amongst other things. > > > > > > This is not ideal for pledged processes, as they often have no way to > > > write files. > > > > > > This changes malloc to use utrace(2) for that. > > > > > > As kdump has no nice way to show those lines without all extras it > > > normally shows, so add two options to it to just show the lines. > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > Run : > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > ... > > > $ kdump -hu > > > > > > Feedback appreciated. > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) is a > good > way to get information outside a pledged process. > > I tend to think it is safe to use it, as the pledged process need cooperation > from outside to exfiltrate informations (a process with permission to call > ktrace(2) on this pid). > > Please note it is a somehow generic problem: at least profiled processes > would > also get advantage of using it. > > > Regarding kdump options, I think that -u option should implies -h (no header). > > Does it would make sens to considere a process using utrace(2) with several > interleaved records for different sources ? A process with MALLOC_OPTIONS=D > and > profiling enabled for example ? An (another) option on kdump to filter on > utrace > label would be useful in such case, or have -u mandate a label to filter on. > > $ MALLOC_OPTIONS=D ktrace -tu your_program > $ kdump -u mallocdumpline > > and for profiling: > > $ kdump -u profil > gmon.out > $ gprof your_program gmon.out > > Thanks. Thanks! Your suggestions make a lot of sense. I'll rework the kdump part to make it more flexable for different purposes. -Otto > -- > Sebastien Marie
