FORUMS
Remove All Ads from XDA

[DEVS-ONLY] SuperSU developer discussion

11,129 posts
Thanks Meter: 84,938
 
By Chainfire, XDA Ad-Free Senior Moderator / Senior Recognized Developer - Where is my shirt? on 5th September 2014, 03:57 PM
Post Reply Email Thread
THIS THREAD IS FOR DEVELOPERS ONLY. IF YOU ARE NOT A DEVELOPER (or very tech-savvy and well-versed), MOST LIKELY YOU SHOULD NOT POST HERE.

By request, I am creating this thread for developer discussion. This is the place for developers to ask questions about how to handle/implement/embed SuperSU, discuss the operation of SuperSU, suggest improvements to compatibility, etc.

This thread hopefully reduces important developer related matters from being buried in the more user-oriented threads.

Please always include the version number of SuperSU you are referring to, even if it's the latest version right now. If applicable, also include information about phone and firmware you are testing with.
The Following 6 Users Say Thank You to Chainfire For This Useful Post: [ View ]
 
 
5th January 2016, 04:28 AM |#2  
garyd9's Avatar
Inactive Recognized Developer
Flag Pittsburgh, PA
Thanks Meter: 2,732
 
More
[DEVS-ONLY] SuperSU developer discussion
Quote:
Originally Posted by Chainfire

The stop-gap solution is to disable this caching completely, which is what the 000000deepsleep script does, which you can find mentioned or quoted in many posts around the forum. From SuperSU 2.66 onwards, that script is automatically installed on Samsung devices when systemless root is used.

Please forgive me for posting (in a cf-auto-root thread) and digging afterwards. I had thought I'd just dump the info and forget about it, but I couldn't stop digging...

...which led me to the quoted material.

Digging in the supersu 2.66 update-scriptbinary, I see that you're detecting "samsung" in the build fingerprint, and if true, doing a systemless install AND applying a deepsleep fix. This works for Galaxy S6 devices, but not for some other similar platform devices. In particular, the Note5 has THREE devices that need caching disabled in order for deep sleep to function. (0:0:0:3 as well as :2 and :1.)

My first question is: does the SGS6 even have a file named "/sys/class/scsi_disk/0:0:0:3/cache_type"? If not, just write to all three files and don't worry about it. The third write would fail on the SGS6 and all would be good. It'd be no worse of a work-around than already exists (and I think it's a bad work-around.)

If that file DOES exist in the SGS6, then something would have caching turned off that really shouldn't. Of course, existing or not, automatically tossing in this deepsleep patch for every single device that has "samsung" in the build fingerprint would seem likely break proper caching in some yet unknown samsung device. Perhaps the SGS7 will change things up so that :1 should be left cache flushable, and :2 would be the only one that should block cache flushing.

As well, it's also possible that Samsung will pull in the kernel fix to resolve this issue before they release Android M. (Okay, perhaps it'd be more likely for Samsung to open source touchwiz... but we can always have fantasies.)

My problem with the work-around is expressed above: it can break something in the future (and cause a support headache when some ONE exynos7420 device is fixed, but the rest aren't.) As well, it sets precedent of having platform specific hacks in the generic update.zip (but only allowing for a single platform and not in an easily expandable way.)

Obviously, it would be a maintenance nightmare to have different "00000deepsleep" files for every different device model. (if 'zerolte.*', SGS6. If 'nobellte.*', Note5, etc.)...

In keeping with what I tell other people, I feel I now have an obligation to suggest A Better Way. (a person shouldn't complain about something unless they can make a reasonable suggestion on how it'd be better.)

So, here's my slightly convoluted (but expandable) suggestion:

You currently use /data/.supersu to read certain variables that modify the supersu.zip installer script. Perhaps those "platform specific lines" could be added to that file, and the installer script would put them in place. So, I could do the following in a recovery root shell before installing supersu.zip:

Code:
echo PLATFORMSTARTUP='echo "temporary none" > /sys/class/scsi_disk/0:0:0:1/cache_type' >> /data/.supersu
(I'd have included both (or all three) needed lines for samsung deep sleep, but I forget how to include CR in a shell cmdline.. )

Then, the supersu installer script would just read PLATFORMSTARTUP and write it's contents to /su/su.d/00000platformstartup (and set perms.)
Given this type of thing, the existing 000000deepsleep hack would be removed. Then, individual devs could easily create simplistic "pre-installers" for supersu for specific platforms that need changes. Those "pre" installers would just write the needed PLATFORMSTARTUP lines to /data/.supersu...

... and then supersu.zip no longer needs platform specific hacks.

Some random XDA developer could then generate a simple "SGS6-supersu.zip" would only contain an edify script to mount /data and add/edit the .supersu file's PLATFORMSTARTUP variable to contain the two lines needed for deep sleep (and another dev could write a Note5 for the 3 lines needed on that platform... and so on..)

Take care
Gary
The Following 4 Users Say Thank You to garyd9 For This Useful Post: [ View ] Gift garyd9 Ad-Free
5th January 2016, 06:16 AM |#3  
Senior Member
Flag Auckland
Thanks Meter: 296
 
More
Quote:
Originally Posted by garyd9

You currently use /data/.supersu to read certain variables that modify the supersu.zip installer script. Perhaps those "platform specific lines" could be added to that file, and the installer script would put them in place. So, I could do the following in a recovery root shell before installing supersu.zip:

...

The only problem with that is that it requires users to have two brain cells to rub together. We've seen time and time again on these forums that you can't assume this is always the case.

I think that Chainfire is doing pretty much the right thing here. At worst, disabling write-back caching will make I/O a bit slower, but that's better than not having deep sleep. The only suggestion I'd have is to add more devices (maybe up to 5), and to check for their existence before writing to them.
5th January 2016, 02:07 PM |#4  
garyd9's Avatar
Inactive Recognized Developer
Flag Pittsburgh, PA
Thanks Meter: 2,732
 
More
Quote:
Originally Posted by NZgeek

The only problem with that is that it requires users to have two brain cells to rub together. We've seen time and time again on these forums that you can't assume this is always the case.

I think that Chainfire is doing pretty much the right thing here. At worst, disabling write-back caching will make I/O a bit slower, but that's better than not having deep sleep.

The problems with the existing solution are:
1. It blindly alters the system kernel behavior for every single device samsung manufactures.
2. It only actually does any good for a single one of the dozens of devices from that sam manufacturer.
3. It completely ignores every OTHER device that might need a bit of help (and potentially does more harm than good for those devices.)
4. It encourages device developers (users on XDA) to download SuperSU.zip and re-package it to have device specific hacks in the .zip archive (creating a mess.)

Actually, I don't think I need to explain all the problems with the existing hack. I'd imagine (hope) that it was done as something quick to test out an idea, and was never intended to be left in place in it's current form.

Quote:
Originally Posted by NZgeek

The only suggestion I'd have is to add more devices (maybe up to 5), and to check for their existence before writing to them.

Which 5 devices? Who maintains that list? Who updates it for each firmware change that might require an update? Will there be a new "SuperSU.zip" package each time a firmware change on a device requires that one of those 5 be changed? Who deals with the support nightmare of saying "use SuperSU v a.bc for device X firmwawre Y" and "superSU v d.ef for device X firmware Z", etc?

My proposed solution takes all the device-specific stuff completely out of the SuperSU package. It changes it from a device-specific solution to be a more generic and expandable solution that requires LESS support from SuperSU and places the device specific burden outside.

Instead of encouraging device developers to repackage supersu to device specific packages, it encourages device developers to package something else alongside supersu that would work with the existing (and unaltered) supersu.zip (and would be future compatible.)

Take care
Gary
The Following 3 Users Say Thank You to garyd9 For This Useful Post: [ View ] Gift garyd9 Ad-Free
8th January 2016, 02:02 PM |#5  
nkk71's Avatar
Recognized Developer / Recognized Contributor
Flag Beirut
Thanks Meter: 6,954
 
Donate to Me
More
Quote:
Originally Posted by spiral777

would there be a way to get kexec/ multirom working with systemless root?

and would flashing a modified boot image to a rom also effect the kexec hardboot partch of the kernel?

1- the current versions of systemless root make changes/additions to the kernel, but you're not "flashing a modified boot img", so kexec is not broken, since the kernel is in essence the same as before

2- yes it is possible for systemless root (tested with 2.65) to get it working on multirom, however some changes were needed; we're still debugging the problem to try to narrow down the issue, to get it to work with as little changes as possible

EDIT: I'll just mention the problems encountered in case @Chainfire wants to be aware of them
a) line 1170: dd if=/dev/zero of=$BOOTIMAGE bs=4096
since MultiROM creates a symlink, the above command actually starts nulling out a "dummy boot.img" file, which basically continues on, untill all free space in internal storage (or external sdcard where applicable) is filled out

b) when MultiROM-TWRP finishes installing SuperSU, the fake /data is still "busy" (some open file or something else keeping it busy), since it's busy, it can't be unmounted properly, and the real mount points don't get restored
at that point mrom injection will fail
using a lazy unmount helped alleviate that (as a workaround), but obviously not the best solution

c) the setprop sukernel.mount 1 (in launch_daemonsu.sh) doesn't trigger the mount properly, workaround was to mount it in the launch_daemonsu.sh using "mount -t ext4 -o loop /data/su.img /su" instead of the setprop

EDIT2: thanks @Captain_Throwback for the reminder
d) the attempted remount read-only, will cause a bootloop; workaround: had to comment that out

just FYI, but I'll check more thoroughly when I get a chance
12th January 2016, 04:41 PM |#6  
Chainfire's Avatar
OP Senior Moderator / Senior Recognized Developer - Where is my shirt?
Thanks Meter: 84,938
 
Donate to Me
More
@garyd9 @NZgeek

Some good points are raised. I am not going to go into them all individually.

There is one core point of disagreement though. While I do not think device-specific patches generally have a place in the SuperSU ZIP installer, the deep sleep issue affects so many million users it is too big to ignore. (By the way, as far as I know this issue affects all recent high-end Samsungs).

While I don't disagree with your ideal of custom pre/post installers, in reality most users will never discover the issue, and just blame SuperSU for suddenly bad battery life. This leads to many support emails, thread posts, bad rep, etc.

Contrast this to for example the LG G3 compatibility patch, which requires the user to indeed write a file to /data or use a pre-installer that does that, the device will simply not boot, which forces the user to either go back to stock, or search for and discover the fix.

Either way, you are right, the patch doesn't even work right for Note users. Thank you for pointing that out - nobody else ever did. I have come up with the following improved script. If for the moment, we put aside our differences regarding the inclusion of any device-specific fixes, what do you think of the following?

It will perform the cache_type change for any scsi_disk, but will skip the ones not set to write protected. This should catch the problem with devices that have a different disk layout, and prevent accidental reduced I/O speed for devices that are not affected.

Note that it is my understanding that the write protection mode cannot be reset without a flash chip power cycle, and as the protection is set by the bootloader long before our check, checking once at boot should suffice.

I would be grateful if you gave that a shot on an affected Note/Edge+ and report back. It successfully sets the cache_type for :1 and :2 on my S6.

Code:
for i in `ls /sys/class/scsi_disk/`; do 
  cat /sys/class/scsi_disk/$i/write_protect 2>/dev/null | grep 1 >/dev/null
  if [ $? -eq 0 ]; then
    echo 'temporary none' > /sys/class/scsi_disk/$i/cache_type
  fi
done
The Following User Says Thank You to Chainfire For This Useful Post: [ View ]
12th January 2016, 05:33 PM |#7  
garyd9's Avatar
Inactive Recognized Developer
Flag Pittsburgh, PA
Thanks Meter: 2,732
 
More
Quote:
Originally Posted by Chainfire

I would be grateful if you gave that a shot on an affected Note/Edge+ and report back. It successfully sets the cache_type for :1 and :2 on my S6.

I won't be able to properly test this until at least tomorrow (Wed) evening... However, in the meantime, the following screenshots suggest that it'd also work on the Note5:

https://goo.gl/photos/61JWzoA5ir3PcDNr9

(This is with a custom kernel, however. I'll post a query in the Note 5 section asking people who are running a stock kernel to run similar commands to post the output here: http://forum.xda-developers.com/show...&postcount=138 - I'll relay the results.)

Let me know when you'd like to debate on if SuperSU should fix (non-root related) bugs in only specific devices, all devices, no devices, or if it should just support a hook to allow third parties to fix both current and future/past devices. (Please don't get the wrong impression from that statement. SuperSU is your product, not mine... However you implement things is up to you.)
The Following User Says Thank You to garyd9 For This Useful Post: [ View ] Gift garyd9 Ad-Free
12th January 2016, 05:54 PM |#8  
Chainfire's Avatar
OP Senior Moderator / Senior Recognized Developer - Where is my shirt?
Thanks Meter: 84,938
 
Donate to Me
More
Please do let me know if I can be of further assistance to fix compatibility.

Quote:
Originally Posted by nkk71

a) line 1170: dd if=/dev/zero of=$BOOTIMAGE bs=4096
since MultiROM creates a symlink, the above command actually starts nulling out a "dummy boot.img" file, which basically continues on, untill all free space in internal storage (or external sdcard where applicable) is filled out

I guess the script can be modified to detect a link and then check if said link is still pointing to /dev/...

Do double symlinks need to be taking into account? i.e. what is a symlink, /dev/block/platform/.../boot, /dev/block/mmcblk0pX, both?

Quote:

b) when MultiROM-TWRP finishes installing SuperSU, the fake /data is still "busy" (some open file or something else keeping it busy), since it's busy, it can't be unmounted properly, and the real mount points don't get restored
at that point mrom injection will fail
using a lazy unmount helped alleviate that (as a workaround), but obviously not the best solution

Complete guesswork, but the backing file may need to be released for the loop device.

Quote:

c) the setprop sukernel.mount 1 (in launch_daemonsu.sh) doesn't trigger the mount properly, workaround was to mount it in the launch_daemonsu.sh using "mount -t ext4 -o loop /data/su.img /su" instead of the setprop

Any idea why?

I'm specifically using the setprop / init.rc way because mount -o loop doesn't work on many firmwares.

Quote:

d) the attempted remount read-only, will cause a bootloop; workaround: had to comment that out

Where is this?
The Following User Says Thank You to Chainfire For This Useful Post: [ View ]
12th January 2016, 06:51 PM |#9  
nkk71's Avatar
Recognized Developer / Recognized Contributor
Flag Beirut
Thanks Meter: 6,954
 
Donate to Me
More
Quote:
Originally Posted by Chainfire

Please do let me know if I can be of further assistance to fix compatibility.

Thank you, I will let you know once I've had a chance to properly debug further

I initially only wanted to get systemless root to work, which using the workarounds (even though not ideal), was proof it can be done
(at the time it was SuperSU v2.65)


Quote:
Originally Posted by Chainfire

I guess the script can be modified to detect a link and then check if said link is still pointing to /dev/...

Do double symlinks need to be taking into account? i.e. what is a symlink, /dev/block/platform/.../boot, /dev/block/mmcblk0pX, both?

No need to take double symlinks into account, only the real one is changed as follows:
the real one is renamed with a "-orig" extension, and a symlink is created to an imaginary normal file:
Code:
cd /dev/block
ls -l
...
brw-------    1 root     root      259,  24 Jan 12 18:18 mmcblk0p40
brw-------    1 root     root      259,  25 Jan 12 18:18 mmcblk0p41
lrwxrwxrwx    1 root     root            67 Jan 12 18:19 mmcblk0p42 -> /realdata/media/0/multirom/roms/HTC_One_M8_GPe_Marshmallo1/boot.img
brw-------    1 root     root      259,  26 Jan 12 18:18 mmcblk0p42-orig
brw-------    1 root     root      259,  27 Jan 12 18:18 mmcblk0p43
...
all other symlinks to the block device remain as is:
Code:
cd /dev/block/platform/msm_sdcc.1/by-name
ls -l
...
lrwxrwxrwx    1 root     root            21 Jan 12 18:18 adsp -> /dev/block/mmcblk0p16
lrwxrwxrwx    1 root     root            20 Jan 12 18:18 board_info -> /dev/block/mmcblk0p3
lrwxrwxrwx    1 root     root            21 Jan 12 18:18 boot -> /dev/block/mmcblk0p42
lrwxrwxrwx    1 root     root            21 Jan 12 18:18 cache -> /dev/block/mmcblk0p46
...


Quote:
Originally Posted by Chainfire

Complete guesswork, but the backing file may need to be released for the loop device.

Will check, thanks.


Quote:
Originally Posted by Chainfire

Any idea why?

I'm specifically using the setprop / init.rc way because mount -o loop doesn't work on many firmwares.

Not really, everything else in init.rc get's executed properly; (and obviously the in launch_daemonsu.sh as well)


Quote:
Originally Posted by Chainfire

Where is this?

at the beginning of launch_daemonsu.sh:
Code:
if [ ! -d "/su/bin" ]; then
  # if we fstab'd system/vendor/oem to rw, remount them ro here
  remount_ro /system
  remount_ro /vendor
  remount_ro /oem
^^ I commented all three of them out, which worked out fine.
MultiROM's secondary ROMs always have system mounted rw, and the above remount_ro will force an immediate reboot


I need to do further testing on these issues, as soon as I come up with something more concrete, I will report back.

EDIT: forgot to mention, can confirm this for the HTC One M7, M8 and M9
The Following User Says Thank You to nkk71 For This Useful Post: [ View ]
13th January 2016, 03:40 AM |#10  
garyd9's Avatar
Inactive Recognized Developer
Flag Pittsburgh, PA
Thanks Meter: 2,732
 
More
Quote:
Originally Posted by garyd9

I'll post a query in the Note 5 section asking people who are running a stock kernel to run similar commands to post the output here: http://forum.xda-developers.com/show...&postcount=138 - I'll relay the results.)

So, two replies. I've edited the results to just show the device and value of write_protect. The first one is a KNOX tripped device with completely stock firmware/kernel (no root):
Code:
scsi_disk/0:0:0:0    0
scsi_disk/0:0:0:1    1
scsi_disk/0:0:0:2    1
scsi_disk/0:0:0:3    1
The second is from a stock device where KNOX has never been tripped (the results are expected, but nice for confirmation):
Code:
scsi_disk/0:0:0:0    0
scsi_disk/0:0:0:1    0
scsi_disk/0:0:0:2    0
scsi_disk/0:0:0:3    0
So far, all indications are that the change suggested would work.

Can I have your permission to modify the superSU 2.66 archive to change the deepsleep script to use the script above and forward it to a couple people to validate? (Or, I can just wait until tomorrow night and do it on my own device.)
13th January 2016, 09:02 AM |#11  
Chainfire's Avatar
OP Senior Moderator / Senior Recognized Developer - Where is my shirt?
Thanks Meter: 84,938
 
Donate to Me
More
Quote:
Originally Posted by garyd9

So far, all indications are that the change suggested would work.

Can I have your permission to modify the superSU 2.66 archive to change the deepsleep script to use the script above and forward it to a couple people to validate? (Or, I can just wait until tomorrow night and do it on my own device.)

Knock yourself out. I'm not in a rush though. I don't expect to release another update for another few days at least.
Post Reply Subscribe to Thread

Guest Quick Reply (no urls or BBcode)
Message:
Previous Thread Next Thread
Thread Tools Search this Thread
Search this Thread:

Advanced Search
Display Modes